Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC4
    • Fix Version/s: 1.5-RC5
    • Component/s: wicket
    • Labels:
      None

      Description

      org.apache.wicket.pageStore.AsynchronousDataStore (ADS) is designed to wrap another IDataStore and save the serialized pages in a different thread.
      The class is only used in unit tests.
      Running org.apache.wicket.page.persistent.disk.DiskDataStoreTest.test1() with ADS seems to be actually slower than running it w/o ADS.
      We need to review the code in ADS and either improve it to scale better or drop this class completely.

      1. async-ds.patch
        9 kB
        Peter Ertl
      2. AsynchronousDataStore.java
        7 kB
        Andrea Del Bene
      3. help-asyncthread.diff
        3 kB
        Antti S. Lankila

        Activity

        Hide
        Martin Grigorov added a comment -

        Closing again after discussing with all authors of patches.

        Show
        Martin Grigorov added a comment - Closing again after discussing with all authors of patches.
        Hide
        Andrea Del Bene added a comment -

        @Martin

        Well, my AsynchronousDataStore seems a bit faster then the current solution. Anaway, I chose LinkedBlockingQueue because in this way I've got just one collection to manage.

        Show
        Andrea Del Bene added a comment - @Martin Well, my AsynchronousDataStore seems a bit faster then the current solution. Anaway, I chose LinkedBlockingQueue because in this way I've got just one collection to manage.
        Hide
        Martin Grigorov added a comment -

        About alankila's patch: I don't like that it keeps a reference to the spawned thread (its runnable) and uses it synchronously without synchronizations.
        About Andrea's patch: I think using blockingQueue is better performing than synchronized LinkedHashMap.
        About Pertl's patch: using LinkedBlockingQueue will grow without upper limit and may lead memory problems. Using Thread.interrupt() may interrupt the thread in the middle of its job (page storing). This may or may not lead to problems later, I'm not sure. I don't see functional problems with the AtomicBoolean flag.

        Some notes on the current impl: It adds entries to the queue up to its capacity. These entries will be saved asynchronously. If there is a heavy load and the queue is full then the entries are added synchronously as they are when AsynchronousDataStore is not in use. This way the user application decides how big queue to use depending on the load they expect and the RAM they can affort.

        Show
        Martin Grigorov added a comment - About alankila's patch: I don't like that it keeps a reference to the spawned thread (its runnable) and uses it synchronously without synchronizations. About Andrea's patch: I think using blockingQueue is better performing than synchronized LinkedHashMap. About Pertl's patch: using LinkedBlockingQueue will grow without upper limit and may lead memory problems. Using Thread.interrupt() may interrupt the thread in the middle of its job (page storing). This may or may not lead to problems later, I'm not sure. I don't see functional problems with the AtomicBoolean flag. Some notes on the current impl: It adds entries to the queue up to its capacity. These entries will be saved asynchronously. If there is a heavy load and the queue is full then the entries are added synchronously as they are when AsynchronousDataStore is not in use. This way the user application decides how big queue to use depending on the load they expect and the RAM they can affort.
        Hide
        Peter Ertl added a comment -

        I would recommend to

        • use Thread#interrupt for background thread termination instead of custom AtomicBoolean
        • use LinkedBlockingQueue instead of ArrayBlockingQueue (this also simplifies the code)

        patch included (file: async-ds.patch)

        Show
        Peter Ertl added a comment - I would recommend to use Thread#interrupt for background thread termination instead of custom AtomicBoolean use LinkedBlockingQueue instead of ArrayBlockingQueue (this also simplifies the code) patch included (file: async-ds.patch)
        Hide
        Andrea Del Bene added a comment -

        Hi,

        I attach a version AsynchronousDataStore which coordinates threads using a synchronized LinkedHashMap. With this type of collection we can iterate through entries in insertion order and we can remove eldest element when we reach max size (see javadoc : http://download.oracle.com/javase/1.5.0/docs/api/java/util/LinkedHashMap.html#removeEldestEntry(java.util.Map.Entry))

        Show
        Andrea Del Bene added a comment - Hi, I attach a version AsynchronousDataStore which coordinates threads using a synchronized LinkedHashMap. With this type of collection we can iterate through entries in insertion order and we can remove eldest element when we reach max size (see javadoc : http://download.oracle.com/javase/1.5.0/docs/api/java/util/LinkedHashMap.html#removeEldestEntry(java.util.Map.Entry ))
        Hide
        Antti S. Lankila added a comment -

        This approach makes the thread that wants to add files to the diskstore help with asyncthread by consuming entries from the async queue until its own work can be appended.

        This should improve throughput during high load conditions and also prevent the case where multiple updates of same pageId become executed out of order (some updates queued could be updated after the direct updates have been performed) because all work to be performed will be done through the queue in the queue insertion order.

        Show
        Antti S. Lankila added a comment - This approach makes the thread that wants to add files to the diskstore help with asyncthread by consuming entries from the async queue until its own work can be appended. This should improve throughput during high load conditions and also prevent the case where multiple updates of same pageId become executed out of order (some updates queued could be updated after the direct updates have been performed) because all work to be performed will be done through the queue in the queue insertion order.
        Hide
        Peter Ertl added a comment -

        reopened since alankila wants to add a patch for more improvement

        Show
        Peter Ertl added a comment - reopened since alankila wants to add a patch for more improvement
        Hide
        Martin Grigorov added a comment -

        I improved the code by not locking globally and use a blocking queue for consumer-producer scenario.
        Please reopen if you think there is a place for further improvements.

        Show
        Martin Grigorov added a comment - I improved the code by not locking globally and use a blocking queue for consumer-producer scenario. Please reopen if you think there is a place for further improvements.

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Martin Grigorov
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development