891541 - OdinMonkey: Ship VTune support with Nightly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

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.

Comment 1

[Joe Olivas]

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.

Comment 2

[Sean Stangl [:sstangl]]

Excellent!

Comment 3

[Luke Wagner [:luke]]

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?

Comment 4

[(no longer active)]

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

Comment 5

[Luke Wagner [:luke]]

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

Comment 6

[Sean Stangl [:sstangl]]

Attachment: Import JITProfiling API and have --enable-profiling imply --enable-vtune where applicable

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.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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)

Comment 8

[Sean Stangl [:sstangl]]

Attachment: Add VTune license to license file

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.

Comment 9

[Mike Hommey [:glandium]]

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

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 11

[Mike Hommey [:glandium]]

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

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 13

[Mike Hommey [:glandium]]

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.

Comment 14

[Gervase Markham [:gerv]]

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

Comment 15

[Mike Hommey [:glandium]]

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'].

Comment 16

[(no longer active)]

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

Comment 17

[Sean Stangl [:sstangl]]

Attachment: [V2] Import JITProfiling API and have --enable-profiling imply --enable-vtune where applicable

Thanks for the review. V2: now with CSRCS, a README, and an explicit wrapper "IsVTuneProfilingActive()".

Comment 18

[Sean Stangl [:sstangl]]

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

Comment 19

[Mike Hommey [:glandium]]

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.

Comment 20

[Mike Hommey [:glandium]]

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

[Gervase Markham [:gerv]]

Comment on attachment 784049 [details] [diff] [review]
[V2] Add VTune BSD license to license file

r=gerv.

Gerv

Comment 22

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c06599384ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58e0d79b024

https://tbpl.mozilla.org/?tree=Try&rev=98ebe43e1d69

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c06599384ad
https://hg.mozilla.org/mozilla-central/rev/b58e0d79b024