Closed Bug 891541 Opened 6 years ago Closed 6 years ago

OdinMonkey: Ship VTune support with Nightly

Categories

(Core :: JavaScript Engine, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: sstangl, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

VTune support for OdinMonkey should be shipped with at least the default Windows Nightly. Although the VTune header files are available under BSD, the library itself cannot be redistributed, so we will need basic runtime library detection.
The library should be redistributable, as the source is available under BSD/GPL dual licensing. The source and license is under <VTune>/sdk/src/ittnotify.
Excellent!
This would be great!  Does nightly build with --enable-profiling and, if so, could we have the VTune code be automatically included when you --enable-profiling?
(In reply to comment #3)
> This would be great!  Does nightly build with --enable-profiling and, if so,
> could we have the VTune code be automatically included when you
> --enable-profiling?

Yes, to the first question.
Great!  I was thinking (since I assume we can only build VTune on Linux/Windows), that, we'd have something roughly like:

#if defined(MOZ_PROFILING) && .. can build VTune ...
# define MOZ_VTUNE
#endif
This patch imports the JITProfiling API from VTune Amplifier XE 2013 Rev 11 into the tree, under a dual GPL/BSD license (so we choose BSD). If --enable-profiling is passed, VTune support will be automatically included on Linux and Windows, which should enable VTune support in Nightly.

Note that VTune support is not hugely exciting until someone drives Bug 873128 to completion. Patch works locally and is currently being digested by Tryserver.
Attachment #783373 - Flags: review?(mh+mozilla)
This is pretty exciting!  Moz folks following along, we have an internal license for vtune if anyone wants to try it out (Windows only though, I can get Linux if there is interest)
I'm assuming here that "choose GPL or BSD" doesn't mean that we just ignore the GPL part of the license in our license file. But I don't know the policy here.
Attachment #783396 - Flags: review?(gerv)
Comment on attachment 783373 [details] [diff] [review]
Import JITProfiling API and have --enable-profiling imply --enable-vtune where applicable

Review of attachment 783373 [details] [diff] [review]:
-----------------------------------------------------------------

r- because i'd rather not see more VPATH.

Can you also add a README file about where the code comes from and how one may update it in the future?

::: js/src/Makefile.in
@@ +108,5 @@
>  # END enclude sources for the Nitro assembler
>  #############################################
>  
> +ifdef MOZ_VTUNE
> +VPATH +=        $(srcdir)/vtune

Could you make it work without an additional VPATH?

::: js/src/moz.build
@@ +328,5 @@
>          'Library.cpp',
>      ]
>  
> +if CONFIG['MOZ_VTUNE']:
> +    CPP_SOURCES += [

CSRCS
Attachment #783373 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #9)
> r- because i'd rather not see more VPATH.

Why?  VPATH is sort of how js is structured right now; adding more is not going to make removing VPATH usage from js/src any more complicated.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #10)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > r- because i'd rather not see more VPATH.
> 
> Why?  VPATH is sort of how js is structured right now; adding more is not
> going to make removing VPATH usage from js/src any more complicated.

IIRC someone is already working on removing VPATH. Adding more bitrots them.
Making this patch will bitrot them anyway, because they'll have to merge it.  You're basically asking sstangl to recreate that work, and he'll likely end up doing it a different way, no?
Comment on attachment 783373 [details] [diff] [review]
Import JITProfiling API and have --enable-profiling imply --enable-vtune where applicable

Turns out bug 888016 didn't do it for C, so let's keep VPATH for now. But please fix the CSRCS thing.
Attachment #783373 - Flags: review- → review+
Comment on attachment 783396 [details] [diff] [review]
Add VTune license to license file

Thanks for asking :-) This is code which actually will ship with Firefox, rather than just being for testing or debugging, right?

If so, then we need to add it - but we will choose to use the code under the BSD terms. So you only need to include the BSD terms in the about:license file (the section "BSD LICENSE", not including that header).

Of course, you should leave the full license header intact on all the source code files.

Gerv
Attachment #783396 - Flags: review?(gerv) → review-
Comment on attachment 783373 [details] [diff] [review]
Import JITProfiling API and have --enable-profiling imply --enable-vtune where applicable

So, I retract my r+, and am back to r- for the same original reason. Turns out bug 888016 *did* handle C files. So you just need to do
CSRCS += ['vtune/jitprofiling.c']

That has the same effect as VPATH += $(srcpath)/vtune and CSRCS += ['jitprofiling.c'].
Attachment #783373 - Flags: review+ → review-
(In reply to comment #14)
> Comment on attachment 783396 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=783396
> Add VTune license to license file
> 
> Thanks for asking :-) This is code which actually will ship with Firefox,
> rather than just being for testing or debugging, right?

This will presumably ship in Nightly, but not other branches.
Thanks for the review. V2: now with CSRCS, a README, and an explicit wrapper "IsVTuneProfilingActive()".
Attachment #783373 - Attachment is obsolete: true
Attachment #784048 - Flags: review?(mh+mozilla)
Thanks. This is the same patch, but with only the BSD section.

VTune support is intended to be included only in Nightly and Aurora releases.
Attachment #783396 - Attachment is obsolete: true
Attachment #784049 - Flags: review?(gerv)
Comment on attachment 784048 [details] [diff] [review]
[V2] Import JITProfiling API and have --enable-profiling imply --enable-vtune where applicable

Review of attachment 784048 [details] [diff] [review]:
-----------------------------------------------------------------

rubberstamp on the vtune imported files. r+ for the rest.
Attachment #784048 - Flags: review?(mh+mozilla) → review+
(In reply to Sean Stangl [:sstangl] from comment #18)
> Created attachment 784049 [details] [diff] [review]
> [V2] Add VTune BSD license to license file
> 
> Thanks. This is the same patch, but with only the BSD section.
> 
> VTune support is intended to be included only in Nightly and Aurora releases.

You could make the license conditional on MOZ_VTUNE (the license.html file is preprocessed). That would mean you need to add the MOZ_TUNE configure.in goo in the main configure.in, though.
Comment on attachment 784049 [details] [diff] [review]
[V2] Add VTune BSD license to license file

r=gerv.

Gerv
Attachment #784049 - Flags: review?(gerv) → review+
https://hg.mozilla.org/mozilla-central/rev/9c06599384ad
https://hg.mozilla.org/mozilla-central/rev/b58e0d79b024
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.