Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3.4
    • Component/s: jackrabbit-core
    • Labels:
      None

      Description

      I've named them Core stats. This will include:

      • number of sessions currently opened
      • session read / write operations per second

      The stats refresh once a minute.
      This is disabled by default, so it will not affect performance.

      1. JCR-3040-v2.patch
        74 kB
        Alex Parvulescu
      2. jr-test.log
        3 kB
        Alex Parvulescu
      3. JCR-3040.patch
        37 kB
        Alex Parvulescu

        Activity

        Hide
        Stefan Guggisberg added a comment -

        the description of JCR-2936 states that
        "There has been a slight interest in the past for adding JMX support.".

        JCR-3040 however does come with a huge commit, affecting critical parts
        of jackrabbit-core, with (IMO) considerable risk of causing regressions.

        i would have appreciated if a patch had been provided for discussion rather
        than committing the changes within 1 minute after creating the issue.

        if there's only 'slight interest' i am rather conservative WRT performing
        major changes in jackrabbit-core.

        Show
        Stefan Guggisberg added a comment - the description of JCR-2936 states that "There has been a slight interest in the past for adding JMX support.". JCR-3040 however does come with a huge commit, affecting critical parts of jackrabbit-core, with (IMO) considerable risk of causing regressions. i would have appreciated if a patch had been provided for discussion rather than committing the changes within 1 minute after creating the issue. if there's only 'slight interest' i am rather conservative WRT performing major changes in jackrabbit-core.
        Hide
        Alex Parvulescu added a comment -

        point taken on the patch.

        I've rolled-back everything, and I'm not re-submitting the JMX Session stats as a patch.
        I've also ran the performance tests (ConcurrentReadTest and ConcurrentReadWriteTest) and attached the results, to analyze the performance impact.

        The problem with these tests is that I ran them on my machine, which I think is error-prone as it is also dependent on the background noise. I've tried to not stress it much (no eclipse running, etc), but I guess it is not 100% bullet proof.
        It would be interesting to run the tests on a continuous integration machine, if there is any available.

        In jr-test.log, you'll find the test results. I ran them 5 times, each time running the 2 versions of the code (with and without my changes to the session state object).

        I'm really interested in your opinion about the test results.

        Show
        Alex Parvulescu added a comment - point taken on the patch. I've rolled-back everything, and I'm not re-submitting the JMX Session stats as a patch. I've also ran the performance tests (ConcurrentReadTest and ConcurrentReadWriteTest) and attached the results, to analyze the performance impact. The problem with these tests is that I ran them on my machine, which I think is error-prone as it is also dependent on the background noise. I've tried to not stress it much (no eclipse running, etc), but I guess it is not 100% bullet proof. It would be interesting to run the tests on a continuous integration machine, if there is any available. In jr-test.log, you'll find the test results. I ran them 5 times, each time running the 2 versions of the code (with and without my changes to the session state object). I'm really interested in your opinion about the test results.
        Hide
        Thomas Mueller added a comment -

        A few remarks:

        I don't really understand the statistics, but it looks like the patch made things 10% slower? Is that so? If yes, I don't understand why. Or did I missinterpret the statistics?

        As far as I understand the code, "opsPerSecond" is the number of operations per second that were called between the first operation and the last operation. Right? For me, that was a bit confusing. I would expect it to mean number of operations divided by the number of seconds the operations took. Otherwise the number of "operations per second" doesn't actually depend on the performance of the write operations, but on how often the application wrote. At least it should be documented:

        CoreStatManagerMBean doesn't contain any Javadocs.

        I would also return the total number of read and the total number of write operations.

        Do you really need to use read and write locks? It seems using volatile fields should be enough, as statistics are not supposed to be completely accurate anyway, but gathering statistics is supposed to be low-overhead (read write locks are not).

        > System.currentTimeMillis() - timeNs / 1000

        I would try to avoid divisions when gathering data, as divisions are slow. Why not use System.nanoTime()? System.currentTimeMillis() is anyway problematic: it can go backwards, as uses the system time and not the elapsed time. Statistics will be completely wrong on summertime change, right?

        > BigDecimal

        I don't really understand why 'double' isn't enough.

        > (non-Javadoc) ...

        What is the reason for adding such comments?

        + if (durationMs == 0)

        { + durationMs = 1000; + }

        I know you want to avoid 'divide by zero' but it might be easier to set the results to 0 instead of changing the duration.

        Show
        Thomas Mueller added a comment - A few remarks: I don't really understand the statistics, but it looks like the patch made things 10% slower? Is that so? If yes, I don't understand why. Or did I missinterpret the statistics? As far as I understand the code, "opsPerSecond" is the number of operations per second that were called between the first operation and the last operation. Right? For me, that was a bit confusing. I would expect it to mean number of operations divided by the number of seconds the operations took. Otherwise the number of "operations per second" doesn't actually depend on the performance of the write operations, but on how often the application wrote. At least it should be documented: CoreStatManagerMBean doesn't contain any Javadocs. I would also return the total number of read and the total number of write operations. Do you really need to use read and write locks? It seems using volatile fields should be enough, as statistics are not supposed to be completely accurate anyway, but gathering statistics is supposed to be low-overhead (read write locks are not). > System.currentTimeMillis() - timeNs / 1000 I would try to avoid divisions when gathering data, as divisions are slow. Why not use System.nanoTime()? System.currentTimeMillis() is anyway problematic: it can go backwards, as uses the system time and not the elapsed time. Statistics will be completely wrong on summertime change, right? > BigDecimal I don't really understand why 'double' isn't enough. > (non-Javadoc) ... What is the reason for adding such comments? + if (durationMs == 0) { + durationMs = 1000; + } I know you want to avoid 'divide by zero' but it might be easier to set the results to 0 instead of changing the duration.
        Hide
        Stefan Guggisberg added a comment -

        thanks for the patch and sharing some test results!

        WRT the results:
        the spread of the test results is huge, i don't think that they allow any conclusion WRT to the potential performance impact.

        WRT the patch:
        i agree with thomas's comments. apart from those i've got a few remarks
        regarding the RepositoryImpl changes:

        • i'd prefer to move the sessionCreated() and sessionLoggedOut() calls outside
          of the 'synchronized (activeSessions) {}' block.
        • MBeanServer.registerMBean() is called on every startup, whether the stats are enabled or not.
          i'd prefer if that call would only be made depending on e.g. a system property.

        WRT to adding JMX support to jackrabbit core:

        just based on 'slight interest' i'm rather reluctant adding major features to jackrabbit core
        with considerable risk of causing regressions.

        Show
        Stefan Guggisberg added a comment - thanks for the patch and sharing some test results! WRT the results: the spread of the test results is huge, i don't think that they allow any conclusion WRT to the potential performance impact. WRT the patch: i agree with thomas's comments. apart from those i've got a few remarks regarding the RepositoryImpl changes: i'd prefer to move the sessionCreated() and sessionLoggedOut() calls outside of the 'synchronized (activeSessions) {}' block. MBeanServer.registerMBean() is called on every startup, whether the stats are enabled or not. i'd prefer if that call would only be made depending on e.g. a system property. WRT to adding JMX support to jackrabbit core: just based on 'slight interest' i'm rather reluctant adding major features to jackrabbit core with considerable risk of causing regressions.
        Hide
        Jukka Zitting added a comment -

        I'm working on tools for better managing and monitoring the repository, and increased JMX support seems like the best way to do this, so I'm in favor of going into this direction.

        However, I agree with the concerns raised here. There's no reason why collecting statistics or exposing them through JMX should have any noticeable impact on performance. If there is a measurable performance impact, then we're doing something wrong.

        Show
        Jukka Zitting added a comment - I'm working on tools for better managing and monitoring the repository, and increased JMX support seems like the best way to do this, so I'm in favor of going into this direction. However, I agree with the concerns raised here. There's no reason why collecting statistics or exposing them through JMX should have any noticeable impact on performance. If there is a measurable performance impact, then we're doing something wrong.
        Hide
        Alex Parvulescu added a comment -

        thanks for all the comments!

        the biggest problem with the initial commit (now patch) was that the impact of the jmx support was unknown in a default scenario where everything is disabled but there still is an impact on the core operations.
        that is what I'm trying to figure out now, as the results of the tests were not clear.

        to me performance with the stats enabled is not an issue in the first iteration, getting a good starting point is. I also needed to start pushing out code to be reviewed, as it piled up on my machine which in the end resulted in a huge commit, and then a huge headache

        @thomas:

        • 10% slower? where did you get the number from? the tests that I ran were inconclusive, see @stefan's comment

        I agree with your observations, but it feels a bit like premature optimization until we actually get the code into JR core (currentTimeMillis vs nanoTime, BigDecimal vs double, locks vs volatile).

        @stefan
        Yes, I'm refactoring to have just one bean (like a dynamic registry) enabled from the start, which would ideally be able to enable other jmx beans on demand.
        I don't like the system property idea, as it kinda defeats the purpose of having jmx support ootb without a restart.

        'slight interest' I should change the description at one point, the priorities constantly change, so should the issue description.
        I also don't see the regression risk, we test constantly for performance degradation on the core parts, plus we'll disable everything ootb. the jmx support has a lot of benefits too, I'm sure in the end it will be worth it.

        @jukka

        • I completely agree on the performance, I still need to run some tests, once we all agree on what the basis of the jmx support will look like (and where it will reside).

        I'll concentrate on building a really low impact ootb jmx support (disabled and with a minimal footprint), then we can measure each component and optimize as needed.

        As we appear to be having a vote for or against jmx support in general, I'll also send an email to the list, to gather some more info on this topic.

        Show
        Alex Parvulescu added a comment - thanks for all the comments! the biggest problem with the initial commit (now patch) was that the impact of the jmx support was unknown in a default scenario where everything is disabled but there still is an impact on the core operations. that is what I'm trying to figure out now, as the results of the tests were not clear. to me performance with the stats enabled is not an issue in the first iteration, getting a good starting point is. I also needed to start pushing out code to be reviewed, as it piled up on my machine which in the end resulted in a huge commit, and then a huge headache @thomas: 10% slower? where did you get the number from? the tests that I ran were inconclusive, see @stefan's comment I agree with your observations, but it feels a bit like premature optimization until we actually get the code into JR core (currentTimeMillis vs nanoTime, BigDecimal vs double, locks vs volatile). @stefan Yes, I'm refactoring to have just one bean (like a dynamic registry) enabled from the start, which would ideally be able to enable other jmx beans on demand. I don't like the system property idea, as it kinda defeats the purpose of having jmx support ootb without a restart. 'slight interest' I should change the description at one point, the priorities constantly change, so should the issue description. I also don't see the regression risk, we test constantly for performance degradation on the core parts, plus we'll disable everything ootb. the jmx support has a lot of benefits too, I'm sure in the end it will be worth it. @jukka I completely agree on the performance, I still need to run some tests, once we all agree on what the basis of the jmx support will look like (and where it will reside). I'll concentrate on building a really low impact ootb jmx support (disabled and with a minimal footprint), then we can measure each component and optimize as needed. As we appear to be having a vote for or against jmx support in general, I'll also send an email to the list, to gather some more info on this topic.
        Hide
        Jukka Zitting added a comment -

        How about using a temporary branch for this to sync up on the code? Once the solution is stable enough we can merge it back to trunk. I'll probably need to work on JMX support (not on stats but more on general JMX bindings as discussed in JCR-2936), so it would be good if we could both work on the same baseline code.

        Show
        Jukka Zitting added a comment - How about using a temporary branch for this to sync up on the code? Once the solution is stable enough we can merge it back to trunk. I'll probably need to work on JMX support (not on stats but more on general JMX bindings as discussed in JCR-2936 ), so it would be good if we could both work on the same baseline code.
        Hide
        Thomas Mueller added a comment -

        > - 10% slower?

        I compared the 50% times. Example: test 4 no changes, ConcurrentReadWriteTest: 1902; with changes: 2317. Well, I don't even know if the numbers are "ms" or "op/s" - you tell me

        > premature optimization

        My comments are about simplicity and not that much about performance. Simplifying the code almost always makes sense. Simplifying early makes a lot of sense, because people tend to be afraid to simplify 'working' code later on.

        Using double instead of BigDecimal just simpler. Avoiding divisions in methods that are called very often also seems logical, specially if the resulting code is actually simpler and better (that is: always correct, which it's not currently). Using volatile versus locks is also about simplicity, plus it avoids nasty deadlocks.

        The rest of my comments are also not really about performance.

        Show
        Thomas Mueller added a comment - > - 10% slower? I compared the 50% times. Example: test 4 no changes, ConcurrentReadWriteTest: 1902; with changes: 2317. Well, I don't even know if the numbers are "ms" or "op/s" - you tell me > premature optimization My comments are about simplicity and not that much about performance. Simplifying the code almost always makes sense. Simplifying early makes a lot of sense, because people tend to be afraid to simplify 'working' code later on. Using double instead of BigDecimal just simpler. Avoiding divisions in methods that are called very often also seems logical, specially if the resulting code is actually simpler and better (that is: always correct, which it's not currently). Using volatile versus locks is also about simplicity, plus it avoids nasty deadlocks. The rest of my comments are also not really about performance.
        Hide
        Stefan Guggisberg added a comment -

        > I also don't see the regression risk, [...]

        how about this one for example?

        <https://builds.apache.org/job/Jackrabbit-trunk/ws/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/jmx/query/QueryStatManager.java>:[107,37] type javax.management.openmbean.OpenType does not take parameters

        i am not a jmx expert but my gut feeling tells me that there could be potential issues with different deployments/environments/etc...

        that's why i don't like ootb jmx support. in your patch you're calling javax.management.* code unconditionally from the RepositoryImpl constructor.
        IMO that should at least be made dependent on configuration.

        Show
        Stefan Guggisberg added a comment - > I also don't see the regression risk, [...] how about this one for example? < https://builds.apache.org/job/Jackrabbit-trunk/ws/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/jmx/query/QueryStatManager.java >: [107,37] type javax.management.openmbean.OpenType does not take parameters i am not a jmx expert but my gut feeling tells me that there could be potential issues with different deployments/environments/etc... that's why i don't like ootb jmx support. in your patch you're calling javax.management.* code unconditionally from the RepositoryImpl constructor. IMO that should at least be made dependent on configuration.
        Hide
        Jukka Zitting added a comment -

        > i am not a jmx expert but my gut feeling tells me that there could be potential issues with different
        > deployments/environments/etc...

        JMX is included as a standard part of Java 5 and higher. The OpenType issue from above is simply a case of using Java 6 features when we should still be sticking with what's included in Java 5.

        Show
        Jukka Zitting added a comment - > i am not a jmx expert but my gut feeling tells me that there could be potential issues with different > deployments/environments/etc... JMX is included as a standard part of Java 5 and higher. The OpenType issue from above is simply a case of using Java 6 features when we should still be sticking with what's included in Java 5.
        Hide
        Bart van der Schans added a comment -

        I would love to have more stats available from jackrabbit and jmx is a good way to expose them imo. I do agree with Stefan that we should have at least a global config option to turn all the stats gathering and/or jmx stuff off.

        Another idea I've been playing with is to expose those stats (and config) also over jcr in the same manner that you have a "/proc" filesystem on linux systems. Once we actually gather the statistic it probably wouldn't be too hard to do that and it could make it really easy to fetch those stats for applications that are already connecting to the repository.

        Show
        Bart van der Schans added a comment - I would love to have more stats available from jackrabbit and jmx is a good way to expose them imo. I do agree with Stefan that we should have at least a global config option to turn all the stats gathering and/or jmx stuff off. Another idea I've been playing with is to expose those stats (and config) also over jcr in the same manner that you have a "/proc" filesystem on linux systems. Once we actually gather the statistic it probably wouldn't be too hard to do that and it could make it really easy to fetch those stats for applications that are already connecting to the repository.
        Hide
        Stefan Guggisberg added a comment -

        > JMX is included as a standard part of Java 5 and higher. The OpenType issue from above is simply a case of using Java 6 features when we should still be sticking with what's included in Java 5.

        sure, however that issue showed up as the first regression right after the commit (QED .

        while jmx is a standard part of java >=5, i guess we'll have to deal with custom MBeanServer implementations depending on the deployment/environment.

        Show
        Stefan Guggisberg added a comment - > JMX is included as a standard part of Java 5 and higher. The OpenType issue from above is simply a case of using Java 6 features when we should still be sticking with what's included in Java 5. sure, however that issue showed up as the first regression right after the commit (QED . while jmx is a standard part of java >=5, i guess we'll have to deal with custom MBeanServer implementations depending on the deployment/environment.
        Hide
        Alex Parvulescu added a comment -

        hi all,

        I'm attaching a reworked version of the JMX support.
        I'll attach it here, I see you like this task better, instead of the parent issue

        The changes:

        • renamed to StatManager, as the core service will provide pure stats, and extra if needed also expose them via jmx. Pure stats should give anybody the freedom they need to build tools the way they need to.
        • there will be a system property 'enableJmxSupport' which will determine if the StatManager should start the jmx Server not
        • the default JMX support will include just one bean called 'DynamicRegistry' (see JmxRegistry for details), this allows anybody to enable jmx support for other Stats.
          So you'll see (for now) 2 methods on the DynamicRegistry: enableCoreStatJmx and enableQueryStatJmx.
        • stats are disabled by default, this has nothing to do with JMX. If you want stats, you'll have to enable them, if you want stats via jmx, enable the jmx support for that Stat object.
        • unit tests to provide a clearer example based on some code.

        Again, I'm not looking for feedback on performance of the code (yet!). The point of this patch was to build with the community's support a good starting point for stats/jmx, and to make sure that collecting stats does not interfere with the raw performance.
        As Stefan suggested, OOTB stats should be disabled, and without any impact on the core. To me this is step one. Step 2 is what happens when you enable the stats

        > sure, however that issue showed up as the first regression right after the commit (QED .
        Stefan, I have to say I'm sad that you would take a Java6 vs Java5 issue and turn it into an argument against jmx support. It is as simple as that, it was (my) tiny mistake, which I fixed after 5 mins of seeing it on the CI server.
        But you have a good point, and now the Jmx is out of the default init of the repository.

        Jukka, a dedicated branch for this effort sound great! How should be proceed?

        Show
        Alex Parvulescu added a comment - hi all, I'm attaching a reworked version of the JMX support. I'll attach it here, I see you like this task better, instead of the parent issue The changes: renamed to StatManager, as the core service will provide pure stats, and extra if needed also expose them via jmx. Pure stats should give anybody the freedom they need to build tools the way they need to. there will be a system property 'enableJmxSupport' which will determine if the StatManager should start the jmx Server not the default JMX support will include just one bean called 'DynamicRegistry' (see JmxRegistry for details), this allows anybody to enable jmx support for other Stats. So you'll see (for now) 2 methods on the DynamicRegistry: enableCoreStatJmx and enableQueryStatJmx. stats are disabled by default, this has nothing to do with JMX. If you want stats, you'll have to enable them, if you want stats via jmx, enable the jmx support for that Stat object. unit tests to provide a clearer example based on some code. Again, I'm not looking for feedback on performance of the code (yet!). The point of this patch was to build with the community's support a good starting point for stats/jmx, and to make sure that collecting stats does not interfere with the raw performance. As Stefan suggested, OOTB stats should be disabled, and without any impact on the core. To me this is step one. Step 2 is what happens when you enable the stats > sure, however that issue showed up as the first regression right after the commit (QED . Stefan, I have to say I'm sad that you would take a Java6 vs Java5 issue and turn it into an argument against jmx support. It is as simple as that, it was (my) tiny mistake, which I fixed after 5 mins of seeing it on the CI server. But you have a good point, and now the Jmx is out of the default init of the repository. Jukka, a dedicated branch for this effort sound great! How should be proceed?
        Hide
        Alex Parvulescu added a comment -

        second iteration of jmx support

        Show
        Alex Parvulescu added a comment - second iteration of jmx support
        Hide
        Jukka Zitting added a comment -

        Let's use the JCR-2936 branch I created for this and other JMX work.

        Show
        Jukka Zitting added a comment - Let's use the JCR-2936 branch I created for this and other JMX work.
        Hide
        Stefan Guggisberg added a comment -

        @alex
        thanks for the reworked patch (JCR-3040-v2.patch).
        looks good so far.

        Show
        Stefan Guggisberg added a comment - @alex thanks for the reworked patch ( JCR-3040 -v2.patch). looks good so far.
        Hide
        Jukka Zitting added a comment -

        The JCR-2936 branch is getting out of date, so I think we should start merging this to trunk. In revision 1180633 I committed the new o.a.j.core.jmx classes from the latest patch. I didn't yet commit the integration bits since I think we still have some work to do there. Most notably I'd rather have the JMX classes depending on other parts of Jackrabbit instead of the other way around.

        Also, to make it easier to collect and expose statistics over a longer period, in revision 1180634 I introduced a simple mechanism for recording a time series using nothing but an AtomicLong instance as the event counter.

        Show
        Jukka Zitting added a comment - The JCR-2936 branch is getting out of date, so I think we should start merging this to trunk. In revision 1180633 I committed the new o.a.j.core.jmx classes from the latest patch. I didn't yet commit the integration bits since I think we still have some work to do there. Most notably I'd rather have the JMX classes depending on other parts of Jackrabbit instead of the other way around. Also, to make it easier to collect and expose statistics over a longer period, in revision 1180634 I introduced a simple mechanism for recording a time series using nothing but an AtomicLong instance as the event counter.
        Hide
        Jukka Zitting added a comment -

        I merged more of the JCR-2936 branch to trunk in revision 1181060.

        Show
        Jukka Zitting added a comment - I merged more of the JCR-2936 branch to trunk in revision 1181060.
        Hide
        Bart van der Schans added a comment -

        Hi,

        About the "integration bits": I spent some time looking at how other (apache) projects like tomcat, camel, cxf, servicemix, etc) try to solve this, but every project seems to have chosen a different solution. Is there some consensus on how we want to go about this? Is there some project that has done this already really well that we can take as example? It make sense to me to spend some time to get this right so we can easily gradually expose more and more over jmx without having to change the "core" code too much.

        Bart

        Show
        Bart van der Schans added a comment - Hi, About the "integration bits": I spent some time looking at how other (apache) projects like tomcat, camel, cxf, servicemix, etc) try to solve this, but every project seems to have chosen a different solution. Is there some consensus on how we want to go about this? Is there some project that has done this already really well that we can take as example? It make sense to me to spend some time to get this right so we can easily gradually expose more and more over jmx without having to change the "core" code too much. Bart
        Hide
        Jukka Zitting added a comment -

        I'm not aware of any best practices beyond the basic JMX documentation, so for now I'd prefer to avoid having too many JMX dependencies inside Jackrabbit core. Ideally the repository itself would just collect all the interesting information and a deployment could then decide whether to make that information available through JMX.

        The ways of making such information available should be flexible enough to allow for new bits to be exposed even without changing the APIs.

        Show
        Jukka Zitting added a comment - I'm not aware of any best practices beyond the basic JMX documentation, so for now I'd prefer to avoid having too many JMX dependencies inside Jackrabbit core. Ideally the repository itself would just collect all the interesting information and a deployment could then decide whether to make that information available through JMX. The ways of making such information available should be flexible enough to allow for new bits to be exposed even without changing the APIs.
        Hide
        Alex Parvulescu added a comment -

        I'll mark this one as fixed since the code is already in and everybody is happy

        Show
        Alex Parvulescu added a comment - I'll mark this one as fixed since the code is already in and everybody is happy

          People

          • Assignee:
            Alex Parvulescu
            Reporter:
            Alex Parvulescu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development