Bug 206859

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.

Please review the attached patch, which introduces new 'sampler' module. Thanks.

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.

(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.

Created attachment 114661 [details]
Updated patch

Updated patch, which should address Y01, Y02, Y03.

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).

*** Bug 207003 has been marked as a duplicate of this bug. ***

[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.

(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.

Created attachment 115516 [details]
Updated patch 2

Updated patch 2, which should address JG01, JG02, JG03, JG04, JG05, JG07

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

(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.

(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.

(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.

Created attachment 115577 [details]
Updated patch 3

Updated patch with Main-Class attribute in sampler module.

(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.

(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.

(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.

(In reply to comment #18)
> I would suggest createManualSampler then.
Thanks, it is a better name.

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

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

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.

(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. :-(

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)

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