JavaScript shell should link against JS shared library

NEW
Unassigned

Status

()

--
enhancement
10 years ago
4 years ago

People

(Reporter: jimb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
With the patch from 433083 applied, the JavaScript shell statically links all the SpiderMonkey object files, instead of linking dynamically against the SpiderMonkey shared library.  This is because the shell refers to js_InitJITStatsClass and jitstats_class, which are hidden symbols defined in jstracer.cpp ("hidden" meaning "visible within object files of the shared library, but not outside the shared library").

Either the interface to the statistics should be improved and made public, or the shell should not use it; either way, the shell should link against the shared library, not contain its own copies of all the library's object files.
(Reporter)

Comment 1

10 years ago
In-person discussion (OMG!) says: just make the JIT stats stuff visible at the linker level, but not in headers.

Updated

10 years ago
Severity: normal → enhancement
A more palatable alternative would be to fix the build system so that we can produce both a static library and a shared library in the same Makefile, and then the shell can link to the static library.
(Reporter)

Comment 3

10 years ago
I can see how that ability might be valuable, but the only reason static linking is relevant to this bug (that I know of) is that we have unresolved questions about what should be exported, and HIDDEN doesn't have its intended effect when using static libraries.  Using that behavior to avoid answering the interface question seems like a non sequitur.
While you should probably sort that out, having a js shell that's linked to a shared libjs will force users to set their library path appropriately, and that seems annoying.

Comment 5

10 years ago
Created attachment 351234 [details] [diff] [review]
changes i needed to build a different js shell
Attachment #351234 - Flags: review?

Updated

10 years ago
Attachment #351234 - Flags: review? → review?(jim)
(Reporter)

Comment 6

10 years ago
Comment on attachment 351234 [details] [diff] [review]
changes i needed to build a different js shell

I don't really feel qualified to review this patch, but I do have some comments, for whatever they're worth:

- Why is the external declaration for resolving_MatchEntry there?  I don't see any new references to it added by your patch, nor any references in js.cpp.

- Cleaning up the interface for the creation of jitstats_class seems like a clearly good thing --- even if we're not going to make it a public API.

- I somehow didn't run into any problems with JS_ObjIsNative, JS_ObjShape, JS_GetScriptAtom, JS_GetSourceNoteSpecName, and JS_GetNextSrcNote.  Why are they needed?  Are these good interfaces? If not, we shouldn't bother adding half-baked interfaces beyond those needed just to build.

- The changes for avoiding js_ScriptClass and js_FunctionClass are kind of kludgy, but they're better than breaking the interface.

- The change to SendSourceToJSDebugger is unrelated, isn't it? Shouldn't that just go in on its own?
Attachment #351234 - Flags: review?(jim)

Comment 7

10 years ago
Created attachment 351343 [details] [diff] [review]
dependent changeset which creates jsdb (not for commit)

while using this changeset to build jsdb on win32, i hit the others

most of my errors were linker errors, things that aren't JS_PUBLIC_API end up being mangled as C++ which causes serious issues.

iirc resolving_MatchEntry is supposed to be extern c by contract, not doing this results in warnings from compilers (or linker errors).
basically

typedef JSBool
(* JSDHashMatchEntry)(JSDHashTable *table, const JSDHashEntryHdr *entry,
                      const void *key);
is in an extern "C" block which means that the type you use for it is supposed to be C, solaris also tends to be very annoyed when you don't follow the rules.

the JS_* things are bits I hit while trying to link js.obj against js3250.dll for jsdb

Comment 8

10 years ago
(In reply to comment #2)
> A more palatable alternative would be to fix the build system so that we can
> produce both a static library and a shared library in the same Makefile, and
> then the shell can link to the static library.
This doesn't help my use case which is that each of js3250.dll and js.exe takes 11.9% of my total build time to link thus linking js.exe against js3250.dll would speed up my build by 13.5%.
(Reporter)

Updated

9 years ago
Duplicate of this bug: 501300
(Reporter)

Comment 10

9 years ago
Note that bug 501300 has some work that is incomplete, but may be useful.
(In reply to comment #8)
> This doesn't help my use case which is that each of js3250.dll and js.exe takes
> 11.9% of my total build time to link thus linking js.exe against js3250.dll
> would speed up my build by 13.5%.

Yeah, this sucks too. Since on Windows we compile with -GL, this means that the linker has to do all the hard work. Thus, every time you link the static library, the linker has to actually compile all that IL into object code. Since we're now linking: a) the DLL, b) js.exe, c) js-tests.exe, the linker gets to do that work 3 times (at > 30 seconds each on my machine).
(Assignee)

Updated

4 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.