Bug 206859 - Extract self sampler into its own module
Extract self sampler into its own module
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: -- Other --
7.0
All All
: P2 (vote)
: 7.2
Assigned To: Tomas Hurka
issues@platform
: API, API_REVIEW_FAST
: 207003 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 08:13 UTC by Tomas Hurka
Modified: 2012-03-03 11:33 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Proposed patch (114.39 KB, patch)
2012-01-03 09:48 UTC, Tomas Hurka
Details | Diff
Updated patch (115.29 KB, patch)
2012-01-05 14:07 UTC, Tomas Hurka
Details | Diff
Updated patch 2 (116.01 KB, patch)
2012-02-08 12:14 UTC, Tomas Hurka
Details | Diff
Updated patch 3 (165.38 KB, patch)
2012-02-10 13:03 UTC, Tomas Hurka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Hurka 2012-01-03 08:13:15 UTC
Currently self-sampling functionality used by slowness detector <http://wiki.netbeans.org/FitnessViaPostMortem> and 'Profile Me' <http://wiki.netbeans.org/FitnessViaPartnership> is part of core.ui module. It would be useful to provide public API for self sampler (currently there is none) and extract self-sampling code into its own module. This way NetBeans Platform applications can use self-sampling without including core.ui module.
Comment 1 Tomas Hurka 2012-01-03 09:48:45 UTC
Created attachment 114571 [details]
Proposed patch

Patch introduces new module 'sampler' and also updates all usages of the self sampler to use new sampler API.
Comment 2 Tomas Hurka 2012-01-03 09:50:42 UTC
Please review the attached patch, which introduces new 'sampler' module. Thanks.
Comment 3 Jaroslav Tulach 2012-01-04 14:23:47 UTC
Y01 Factory method should be called createXYZ rather than getXYZ

Y02 apichanges.xml should mention initial release, so we have an entry in changes for 7.2

Y03 Official APIs should be in org.netbeans.api package

Y04 I am not sure I really understand the difference between sampler and generic sampler. Definitely not from Javadoc of its factory methods.
Comment 4 Tomas Hurka 2012-01-05 14:05:39 UTC
(In reply to comment #3)
> Y01 Factory method should be called createXYZ rather than getXYZ
Sure, I Will change it.
 
> Y02 apichanges.xml should mention initial release, so we have an entry in
> changes for 7.2
Hopefully done.
 
> Y03 Official APIs should be in org.netbeans.api package
After an off-line discussion with Jarda, I would like to change the API category to "standard". My main motivation for the change was the reason, that this API just helps debug NetBeans Platform applications, it does not provide any user level functionality.

> Y04 I am not sure I really understand the difference between sampler and
> generic sampler. Definitely not from Javadoc of its factory methods.
I think that the javadoc is OK, but I agree that the method names can be better, especially genericSampler. Does anybody have some ideas?

Updated patch will be attached shortly.
Comment 5 Tomas Hurka 2012-01-05 14:07:15 UTC
Created attachment 114661 [details]
Updated patch

Updated patch, which should address Y01, Y02, Y03.
Comment 6 Jaroslav Tulach 2012-01-05 17:50:16 UTC
Re. Y04: I still think the javadoc is not clear. It feels to me it concentrates on wrong message/topic. Maybe it is because it mixes the specification (what the method will do) and non-normative usage (what it should be used for) and starts with non-normative usage. If it was so, then there is a <div class="non-normative">...</div> which is used to give the "for what" part a different background.

Yes, names of the methods are confusing. If it was up to me, I'd:

a) create a single method create(String name, boolean onlyIfSafe);

or 

b) have create(String name) and boolean isAutomaticSamplingSafe();

but I know Tomáš does not agree on (a) neither (b).
Comment 7 Jesse Glick 2012-01-06 22:19:55 UTC
*** Bug 207003 has been marked as a duplicate of this bug. ***
Comment 8 Jesse Glick 2012-01-06 22:44:34 UTC
[JG01] I would suggest running the convert to @Messages hint before undertaking a refactoring like this, so you can be sure you are not leaving behind orphaned bundle keys. And SAVE_MSG could be simplified to a generated method call. But up to you.


[JG02] If the "logger-" prefix is universal, then perhaps it should be dropped, since it seems to have been useful just in order to distinguish magic Action attributes from other stuff. I.e. just:

this.profiler = Sampler.createSampler("completion");


[JG03] typo: "Sapler API"


[JG04] Use Places, not System.getProperty("netbeans.user").


[JG05] defaultDataObject hack could perhaps be replaced with a check of fo.getMIMEType().equals("content/unknown").


[JG06] I like Y04a. It is not clear from Javadoc that the two factory methods pretty much produce the same thing, just depending on whether or not you are in a safe run mode.


[JG07] Should not have public methods which are neither final nor abstract; it is not clear whether they may be overridden or not.
Comment 9 Tomas Hurka 2012-02-08 12:07:44 UTC
(In reply to comment #8)
> [JG01] I would suggest running the convert to @Messages hint before undertaking
> a refactoring like this, so you can be sure you are not leaving behind orphaned
> bundle keys. And SAVE_MSG could be simplified to a generated method call. But
> up to you.
Right, unfortunately it is too late to do it before this change. So I converted new bundle keys in sampler module to use @Messages.

> [JG02] If the "logger-" prefix is universal, then perhaps it should be dropped,
> since it seems to have been useful just in order to distinguish magic Action
> attributes from other stuff. I.e. just:
> 
> this.profiler = Sampler.createSampler("completion");
Yes, the logger- string was a relict. I removed it and added "sampler-" prefix to all names, so that timer threads created by sampler can be easily detected.

> [JG03] typo: "Sapler API"
Fixed.

> [JG04] Use Places, not System.getProperty("netbeans.user").
Done, but it is a pity to add new module dependency for just this one usage. BTW: I was a little bit surprised that I need a dependency on org.openide.modules to get userdir.

> [JG05] defaultDataObject hack could perhaps be replaced with a check of
> fo.getMIMEType().equals("content/unknown").
Yes, this seems to work fine.

> [JG06] I like Y04a. It is not clear from Javadoc that the two factory methods
> pretty much produce the same thing, just depending on whether or not you are in
> a safe run mode.
That it produce a pretty much same thing is a implementation detail. I still see it as two different factories. With the boolean flag the usages would look like:

his.profiler = Sampler.createSampler("completion",true);
or
his.profiler = Sampler.createSampler("completion",false);

but when you look at it, it is not clear what the does the true/false mean.

> [JG07] Should not have public methods which are neither final nor abstract; it
> is not clear whether they may be overridden or not.
Sure, fixed.

Updated patch will be attached shortly.
Comment 10 Tomas Hurka 2012-02-08 12:14:00 UTC
Created attachment 115516 [details]
Updated patch 2

Updated patch 2, which should address JG01, JG02, JG03, JG04, JG05, JG07
Comment 11 Jaroslav Tulach 2012-02-09 09:50:47 UTC
Y05: I'd suggest to also add Main-Class attribute to the manifest and modify http://wiki.netbeans.org/FitnessViaCLITimeLine to describe 7.2 dev build accordingly
Comment 12 Tomas Hurka 2012-02-09 10:00:14 UTC
(In reply to comment #11)
> Y05: I'd suggest to also add Main-Class attribute to the manifest 
ok, but org-netbeans-core-ui.jar does not have it.

> and modify
> http://wiki.netbeans.org/FitnessViaCLITimeLine to describe 7.2 dev build
> accordingly
Yes, I had that in mind.
Comment 13 Jaroslav Tulach 2012-02-09 10:47:12 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Y05: I'd suggest to also add Main-Class attribute to the manifest 
> ok, but org-netbeans-core-ui.jar does not have it.

I know and every time I tried to use it, I needed to go to the wiki page to find out the name of the class to call. With Main-Class, it would be enough to remember which JAR does the work. In case of your new sampler JAR, it will be safe bet as the name will be encoded in the JAR.
Comment 14 Tomas Hurka 2012-02-10 08:53:32 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Y05: I'd suggest to also add Main-Class attribute to the manifest 
> > ok, but org-netbeans-core-ui.jar does not have it.
> 
> I know and every time I tried to use it, I needed to go to the wiki page to
> find out the name of the class to call. With Main-Class, it would be enough to
> remember which JAR does the work. In case of your new sampler JAR, it will be
> safe bet as the name will be encoded in the JAR.
I understand. My only concern is that it is a little bit confusing for netbeans module to have a main-class.
Comment 15 Tomas Hurka 2012-02-10 13:03:34 UTC
Created attachment 115577 [details]
Updated patch 3

Updated patch with Main-Class attribute in sampler module.
Comment 16 Jesse Glick 2012-02-13 21:07:06 UTC
(In reply to comment #9)
> That it produce a pretty much same thing is a implementation detail.

Then how exactly do you foresee the implementation diverging in the future?

> I still see it as two different factories.

It is returning the exact same InternalSampler object in either case! Only the conditions under which it can return null vary.

> his.profiler = Sampler.createSampler("completion",true);
> or
> his.profiler = Sampler.createSampler("completion",false);
> 
> when you look at it, it is not clear what the true/false means

It will be clear when the boolean param is named something like "inRunModeOnly" or "manuallyInvoked". The current createSampler(String) is fine as far as naming is concerned but "createGenericSampler" means nothing to me. In what sense is this sampler "generic" while the other one is "specific"?

Even the implementation is confused. Both variants effectively first check SamplesOutputStream.isSupported() - though the Javadoc of createSampler fails to mention that it can also return null if this fails - and either check isRunMode or not and then call the constructor, but they do so using visually different idioms.
Comment 17 Tomas Hurka 2012-02-23 16:32:15 UTC
(In reply to comment #16)
> (In reply to comment #9)
> > That it produce a pretty much same thing is a implementation detail.
> 
> Then how exactly do you foresee the implementation diverging in the future?
I am not sure exactly, but createSampler(String) could also eliminate overhead from other applications running on the same host or alternatively it can return null in such cases.
 
> > I still see it as two different factories.
> 
> It is returning the exact same InternalSampler object in either case! Only the
> conditions under which it can return null vary.
Currently yes, but the usage of those two methods are quite different. The one used for automatic reporting (createSampler) should try to strip off all the external influences. Currently if the process is running under debugger or profiler it returns null, which means it cannot do meaningful measurement in such situation. In the future it can also consider external applications which could impact the measurement. It could either return null or in better implementation compensate such external impact. The second method for manual sampling (createGenericSampler) is invoked by user and should be available in all cases and reflect current state, so you can sample debugged or profiled NetBeans or sample NetBeans, where the whole OS is under heavy load. 

> > his.profiler = Sampler.createSampler("completion",true);
> > or
> > his.profiler = Sampler.createSampler("completion",false);
> > 
> > when you look at it, it is not clear what the true/false means
> 
> It will be clear when the boolean param is named something like "inRunModeOnly"
> or "manuallyInvoked". The current createSampler(String) is fine as far as
> naming is concerned but "createGenericSampler" means nothing to me. In what
> sense is this sampler "generic" while the other one is "specific"?
As I said above, the name is not very nice and if we can come up with better one, I will be happy. So how do I come to the generic name? Well, the generic here means that it can be used in all situations, which means that it returns non-null in most of cases. The only exception is some obscure JVM implementation. The "specific" means that it is designed for the automatic sampling. Since I expect that the automatic sampling will be used on several places through the NetBeans (and already it is) while user invoked sampling is in just in one place, the name for automatic sampling is the one, which comes to one mind, when you want to get Sampler object.
 
> Even the implementation is confused. 
No, the implementation is ok.

> Both variants effectively first check
> SamplesOutputStream.isSupported() - though the Javadoc of createSampler fails
> to mention that it can also return null if this fails - and either check
> isRunMode or not and then call the constructor, but they do so using visually
> different idioms.
Javadoc can be updated with notice that it can also return null on exotic JVM, but since the users of the API should already check the return value for null, I don't think this is important.
Comment 18 Jesse Glick 2012-02-27 19:31:18 UTC
(In reply to comment #17)
> the generic [term]
> here means that it can be used in all situations, which means that it returns
> non-null in most of cases. The only exception is some obscure JVM
> implementation. The "specific" means that it is designed for the automatic
> sampling.

I would suggest createManualSampler then.
Comment 19 Tomas Hurka 2012-02-29 14:16:42 UTC
(In reply to comment #18)
> I would suggest createManualSampler then.
Thanks, it is a better name.
Comment 20 Tomas Hurka 2012-02-29 17:21:42 UTC
Integrated into profiler-main with updated javadoc for createSampler() and new method name createManualSampler()


changeset:   213897:486659ee520f
user:        Tomas Hurka <thurka@netbeans.org>
date:        Wed Feb 29 15:47:14 2012 +0100
summary:     issue #206859 extract self sampler into its own module
Comment 21 Vladimir Voskresensky 2012-03-02 15:35:44 UTC
after that build fails with:

    [mkdir] Created dir: /export1/hudson/jobs/cnd-build/workspace/uihandler/build/test/unit/classes
 [nb-javac] Compiling 27 source files to /export1/hudson/jobs/cnd-build/workspace/uihandler/build/test/unit/classes
[subant-junit] /export1/hudson/jobs/cnd-build/workspace/uihandler/test/unit/src/org/netbeans/modules/uihandler/SelfSampleVFSTest.java:51: cannot find symbol
[subant-junit] symbol  : class SelfSampleVFSTest
[subant-junit] location: package org.netbeans.core.ui.sampler
[subant-junit] public class SelfSampleVFSTest extends org.netbeans.core.ui.sampler.SelfSampleVFSTest {
[subant-junit]                                                                    ^
[subant-junit] Note: Attempting to workaround javac bug #6512707
[subant-junit] /export1/hudson/jobs/cnd-build/workspace/uihandler/test/unit/src/org/netbeans/modules/uihandler/SelfSampleVFSTest.java:51: cannot find symbol
[subant-junit] symbol  : class SelfSampleVFSTest
[subant-junit] location: package org.netbeans.core.ui.sampler
[subant-junit] public class SelfSampleVFSTest extends org.netbeans.core.ui.sampler.SelfSampleVFSTest {
[subant-junit]                                                                    ^
[subant-junit] /export1/hudson/jobs/cnd-build/workspace/uihandler/test/unit/src/org/netbeans/modules/uihandler/SelfSampleVFSTest.java:57: method does not override or implement a method from a supertype
[subant-junit]     @Override
[subant-junit]     ^
[subant-junit] 2 errors
Comment 22 Petr Jiricka 2012-03-02 16:59:12 UTC
It looks like this was propagated to other team repositories because the profiler-main build does not compile unit tests and allows propagation of changes even if test compilation fails. The http://deadlock.netbeans.org/hudson/job/profiler-main/ build should invoke the build-test-dist target.
Comment 23 Tomas Hurka 2012-03-02 17:07:40 UTC
(In reply to comment #22)
> It looks like this was propagated to other team repositories because the
> profiler-main build does not compile unit tests and allows propagation of
> changes even if test compilation fails. The
> http://deadlock.netbeans.org/hudson/job/profiler-main/ build should invoke the
> build-test-dist target.

The main problem is that push-profiler-main is be invoked manually and I did not invoke it, so it should not be in profiler-silver. The problem is caused by the migration deadlock, where push-profiler-main is probably set to run automatically. :-(
Comment 24 Tomas Hurka 2012-03-02 17:22:19 UTC
I am sorry for the broken test. Fixed in main <http://hg.netbeans.org/main/rev/0104a88f73c1>

changeset:   214190:0104a88f73c1
user:        Tomas Hurka <thurka@netbeans.org>
date:        Fri Mar 02 18:16:38 2012 +0100
summary:     issue #206859 extract self sampler into its own module (fix in uihandler)
Comment 25 Quality Engineering 2012-03-03 11:33:10 UTC
Integrated into 'main-golden', will be available in build *201203030400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/486659ee520f
User: Tomas Hurka <thurka@netbeans.org>
Log: issue #206859 extract self sampler into its own module


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo