Felix
  1. Felix
  2. FELIX-908

Unsynchronize access to bundle state inside BundleInfo by making the variable volatile

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: framework-1.2.1
    • Fix Version/s: framework-1.6.0
    • Component/s: Framework
    • Labels:
      None

      Description

      Synchronized access to bundle state inside BundleInfo class creates an unnecessary contention point in environments where bundle.getState() is called a lot.
      Declaring m_state variable volatile and removing synchronization from setState() and getState() will do the trick.I will attach the patch.

      1. bundle_info.patch
        0.9 kB
        Anatoli Kazatchkov

        Activity

        Hide
        Felix Meschberger added a comment -

        IIRC assignment of an int is an atomic operation (in contrast to long assignment which is not atomic), so synchronization is not required at, either the state field has the new or the old value, but never something in-between.

        Also IIRC, the handling of volatile fields has changed with the Java 5 memory model. So using a volatile field here might create an implicit dependency on Java 5, right ?

        I may be completely wrong here

        Show
        Felix Meschberger added a comment - IIRC assignment of an int is an atomic operation (in contrast to long assignment which is not atomic), so synchronization is not required at, either the state field has the new or the old value, but never something in-between. Also IIRC, the handling of volatile fields has changed with the Java 5 memory model. So using a volatile field here might create an implicit dependency on Java 5, right ? I may be completely wrong here
        Hide
        Stuart McCulloch added a comment -

        A plain int field is still different from a volatile int, because in the Java Memory Model different threads could see different values in the same int field because of caching, etc. (of course in reality this might never happen, it depends on the actual architecture of the OS, etc.) Marking a field as volatile implies a memory fence, and also stops the JIT from re-ordering statements, which could also break assumptions about the field. So there is a difference, but it's not always obvious.

        Pre-Java 5 this was all a bit of a mess, and many JVMs didn't support volatile longs properly - with Java 5 things are much better, and the semantics are cleaner.
        See http://jeremymanson.blogspot.com/2008/11/what-volatile-means-in-java.html for a nice overview and http://gee.cs.oswego.edu/dl/jmm/cookbook.html for the gory details

        Getting back to the issue, using volatile doesn't imply a dependency on Java 5 - unless you're relying on specific assumptions (like the double-checked locking idiom). We already use volatile in other places in Felix that have high contention, the question here is whether using volatile will break the bundle state machine - will need to look more closely at this code to be sure. ie. if only one thread is writing to the state and all other threads read it then volatile is fine, the problem is if two or more threads need to both read and write to it concurrently.

        Show
        Stuart McCulloch added a comment - A plain int field is still different from a volatile int, because in the Java Memory Model different threads could see different values in the same int field because of caching, etc. (of course in reality this might never happen, it depends on the actual architecture of the OS, etc.) Marking a field as volatile implies a memory fence, and also stops the JIT from re-ordering statements, which could also break assumptions about the field. So there is a difference, but it's not always obvious. Pre-Java 5 this was all a bit of a mess, and many JVMs didn't support volatile longs properly - with Java 5 things are much better, and the semantics are cleaner. See http://jeremymanson.blogspot.com/2008/11/what-volatile-means-in-java.html for a nice overview and http://gee.cs.oswego.edu/dl/jmm/cookbook.html for the gory details Getting back to the issue, using volatile doesn't imply a dependency on Java 5 - unless you're relying on specific assumptions (like the double-checked locking idiom). We already use volatile in other places in Felix that have high contention, the question here is whether using volatile will break the bundle state machine - will need to look more closely at this code to be sure. ie. if only one thread is writing to the state and all other threads read it then volatile is fine, the problem is if two or more threads need to both read and write to it concurrently.
        Hide
        Karl Pauls added a comment -

        Making the member volatile and removing the synchronization is fine in this case (we only have on writer at a time).

        Show
        Karl Pauls added a comment - Making the member volatile and removing the synchronization is fine in this case (we only have on writer at a time).
        Hide
        Felix Meschberger added a comment -

        Thanks Stuart to enlighten (and correct) me.

        Show
        Felix Meschberger added a comment - Thanks Stuart to enlighten (and correct) me.
        Hide
        Richard S. Hall added a comment -

        Out of curiosity, what scenario is causing Bundle.getState() to get invoked so often?

        Show
        Richard S. Hall added a comment - Out of curiosity, what scenario is causing Bundle.getState() to get invoked so often?
        Hide
        Don Brown added a comment -

        The cause is how our plugin system interacts with OSGi, not so much as a technical requirement but legacy design decision. There are a number of situations where the plugin system will iterate through every plugin (which may or may not be an OSGi bundle) looking for something, with the first call going to isEnabled(), where the bundle state check happens for OSGi plugins.

        Show
        Don Brown added a comment - The cause is how our plugin system interacts with OSGi, not so much as a technical requirement but legacy design decision. There are a number of situations where the plugin system will iterate through every plugin (which may or may not be an OSGi bundle) looking for something, with the first call going to isEnabled(), where the bundle state check happens for OSGi plugins.
        Hide
        Richard S. Hall added a comment -

        I committed a patch to trunk to make access to bundle state volatile. Please close this issue if you are satisfied.

        Show
        Richard S. Hall added a comment - I committed a patch to trunk to make access to bundle state volatile. Please close this issue if you are satisfied.

          People

          • Assignee:
            Richard S. Hall
            Reporter:
            Anatoli Kazatchkov
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development