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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(8 attachments)

35.40 KB, patch
jandem
: review+
Details | Diff | Splinter Review
9.18 KB, patch
jandem
: review+
Details | Diff | Splinter Review
8.35 KB, patch
jandem
: review+
Details | Diff | Splinter Review
9.31 KB, patch
jandem
: review+
Details | Diff | Splinter Review
18.47 KB, patch
jandem
: review+
Details | Diff | Splinter Review
15.82 KB, patch
jandem
: review+
Details | Diff | Splinter Review
26.43 KB, patch
jandem
: review+
Details | Diff | Splinter Review
15.37 KB, patch
jandem
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #9004321 - Flags: review?(jdemooij)
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.
Attachment #9004325 - Flags: review?(jdemooij)
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.
Attachment #9004326 - Flags: review?(jdemooij)
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.
Attachment #9004332 - Flags: review?(jdemooij)
Header name is a bit long, but it works.  Open to suggestions for simpler names.
Attachment #9004333 - Flags: review?(jdemooij)
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.
Attachment #9004339 - Flags: review?(jdemooij)
Attachment #9004321 - Flags: review?(jdemooij) → review+
Attachment #9004325 - Flags: review?(jdemooij) → review+
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
Attachment #9004326 - Flags: review?(jdemooij) → review+
Attachment #9004332 - Flags: review?(jdemooij) → review+
Attachment #9004333 - Flags: review?(jdemooij) → review+
Attachment #9004335 - Flags: review?(jdemooij) → review+
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?
Attachment #9004339 - Flags: review?(jdemooij) → review+
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.
Attachment #9004340 - Flags: review?(jdemooij) → review+
Pushed by jwalden@mit.edu:
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
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.
Flags: needinfo?(jwalden+bmo)
(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
Flags: needinfo?(jwalden+bmo)
Pushed by jwalden@mit.edu:
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
You need to log in before you can comment on or make changes to this bug.