1486577 - Move all compilation APIs to a header independent of jsapi.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

I'm mostly lost trying to find/enumerate them all in jsapi.h as it is now.  Plus this helps to make clearer just who's ever compiling/evaluating scripts.

This also probably will break every JSAPI client out there by requiring a new header location.  :-)  No pain, no gain.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Move all compile-options classes to a new public js/CompileOptions.h header

I have chosen to over-break things out so that header sizes can be smaller.  That means a couple small concepts that are essential to compilation, are in their own headers, when you *could* imagine them in a single compilation-related header.  I think that helps with readability, as with this header.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Move JS::SourceBufferHolder into a new public js/SourceBufferHolder.h header

It's worth noting that everything takes this class as reference, so at the end of these patches, all header-users can just rely on forward-declarations, and all actual-users can #include the header -- an extra benefit over forcing everyone to have this definition around, by putting it in the central compilation-and-evaluation header.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Move script/function transcoding API into a new public js/TranscodeScript.h header

I'm not wholly satisfied with the name of this header, because the "script" part of it is a little too JSAPI-specific IMO.  Suggestions for anything better are appreciated.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Move of-thread compilation API into a new public js/OffThreadScriptCompilation.h header

Aside from minimizing header size, IMO this is good because compiling off-thread is probably not the common case, and so people looking at headers don't even have to see this complexity if we shunt it into another header.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Move compilation and evaluation APIs into a new public js/CompilationAndEvaluation.h header

Header name is a bit long, but it works.  Open to suggestions for simpler names.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Don't #include js/SourceBufferHolder.h in jsapi.h, and instead require users to do so -- a translation-unit size improvement for anyone who never has to use SourceBufferHolder other than by reference

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Make all users of the various *CompileOptions classes #include "js/CompileOptions.h" so that nothing but that file has to know about these classes having a JS_PUBLIC_API on them, that would have to be present in forward-declarations

Forward-declaring anything that's JS_PUBLIC_API or JS_FRIEND_API is fraught with peril depending on the compiler, so it's best not to do so if possible.  IMO with the CompileOptions dependencies minimized now, we should just make everyone use the header that actually defines them all.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Don't #include "js/CompilationAndEvaluation.h" in jsapi.h, minimizing the scope of that header and reducing translation-unit size of anything that needs JSAPI but doesn't need to compile/evaluate JavaScript

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9004326 [details] [diff] [review]
Move script/function transcoding API into a new public js/TranscodeScript.h header

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

Maybe TranscodeAPI.h or Transcoding.h? (I wanted to suggest XdrAPI.h but that's probably too much SM-jargon.) Else I'd have a slight preference for s/TranscodeScript.h/ScriptTranscoding.h/, it's a bit similar to CharacterEncoding.h

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 9004339 [details] [diff] [review]
Make all users of the various *CompileOptions classes #include "js/CompileOptions.h" so that nothing but that file has to know about these classes having a JS_PUBLIC_API on them, that would have to be present in forward-declarations

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

::: js/public/CompileOptions.h
@@ +214,4 @@
>      void operator=(const ReadOnlyCompileOptions&) = delete;
>  };
>  
> +class JS_PUBLIC_API(CompileOptions);

Part 1 has:

class MOZ_STACK_CLASS JS_FRIEND_API(CompileOptions) final

Should we change that to JS_PUBLIC_API as well?

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 9004340 [details] [diff] [review]
Don't #include "js/CompilationAndEvaluation.h" in jsapi.h, minimizing the scope of that header and reducing translation-unit size of anything that needs JSAPI but doesn't need to compile/evaluate JavaScript

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

Nice patches.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b77a399fcf
Move all compile-options classes to a new public js/CompileOptions.h header.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8351c63f54
Move JS::SourceBufferHolder into a new public js/SourceBufferHolder.h header.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa263f9eda3
Move script/function transcoding API into a new public js/Transcoding.h header.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8896f3fdaf99
Move of-thread compilation API into a new public js/OffThreadScriptCompilation.h header.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb317294f45
Move compilation and evaluation APIs into a new public js/CompilationAndEvaluation.h header.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc7007e3924
Don't #include js/SourceBufferHolder.h in jsapi.h, and instead require users to do so -- a minor translation-unit size improvement for anyone who never has to use SourceBufferHolder other than by reference.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb7ddfad86d
Make all users of the various *CompileOptions classes #include "js/CompileOptions.h" so that nothing but that file has to know about these classes having a JS_PUBLIC_API on them, that would have to be present in forward-declarations.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/48921866b394
Don't #include "js/CompilationAndEvaluation.h" in jsapi.h, minimizing the scope of that header and reducing translation-unit size of anything that needs JSAPI but doesn't need to compile/evaluate JavaScript.  r=jandem

Comment 13

[Paul Bone [:pbone]]

Hi Jeff, I'm getting a build error for the shell with clang-5.0

In file included from /home/paul/dev/moz/dont-poison/js/src/vm/TraceLogging.cpp:17:
In file included from /home/paul/dev/moz/dont-poison/js/src/jsapi.h:31:
/home/paul/dev/moz/dont-poison/js/src/b-assert/dist/include/js/CompileOptions.h:165:7: error: visibility does not match previous declaration
class JS_PUBLIC_API(ReadOnlyCompileOptions)
      ^
/home/paul/dev/moz/dont-poison/js/src/jstypes.h:47:30: note: expanded from macro 'JS_PUBLIC_API'
#  define JS_PUBLIC_API(t)   MOZ_EXPORT t
                             ^
/home/paul/dev/moz/dont-poison/js/src/b-assert/dist/include/mozilla/Types.h:44:45: note: expanded from macro 'MOZ_EXPORT'
#    define MOZ_EXPORT       __attribute__((visibility("default")))
                                            ^
/home/paul/dev/moz/dont-poison/config/gcc_hidden.h:6:13: note: previous attribute is here
#pragma GCC visibility push(hidden)
            ^


It appears to be caused by this change.

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/31b77a399fcf
https://hg.mozilla.org/mozilla-central/rev/ea8351c63f54
https://hg.mozilla.org/mozilla-central/rev/daa263f9eda3
https://hg.mozilla.org/mozilla-central/rev/8896f3fdaf99
https://hg.mozilla.org/mozilla-central/rev/9fb317294f45
https://hg.mozilla.org/mozilla-central/rev/2dc7007e3924
https://hg.mozilla.org/mozilla-central/rev/1fb7ddfad86d
https://hg.mozilla.org/mozilla-central/rev/48921866b394

Comment 15

[Steve Fink [:sfink] [:s:]]

(In reply to Paul Bone [:pbone] from comment #13)
> Hi Jeff, I'm getting a build error for the shell with clang-5.0

Fixed by bbouvier in bug 1486731, 
http://hg.mozilla.org/integration/mozilla-inbound/rev/5202cfbf8d60

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b954ab9d43
Remove a spurious inclusion of js/CompileOptions.h from vm/TraceLogging.h, because the header doesn't need any *CompileOptions declaration or definition.  r=me as trivial, following up on a bustage fix that got merged into the landing for bug 1486731

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/23b954ab9d43