1275639 - Ensure we use the same set of configure flags for SM(pkg) that Servo/rust-mozjs use

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Follow up to bug 1273917.

We should verify that we are using the same set of configure flags for SM(pkg) that Servo and rust-mozjs uses.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We are not; Servo uses `--disable-shared-js` (see <https://github.com/servo/mozjs/blob/master/makefile.cargo>), which is broken in m-c. Allegedly glandium is working on fixing that.

Comment 2

[Mike Hommey [:glandium]]

The configure flags should have no effect on the source tarballs.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The pkg build also extracts the tarball and builds from it.

Comment 4

[Mike Hommey [:glandium]]

That's useless if you ask me. If you want a build job that validates that sm builds fine for servo, you need one that runs on all platforms.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

A partial solution is just that: partial, not useless.

Comment 6

[Mike Hommey [:glandium]]

It's also bloating a job whose description has nothing to do with that. See, I thought those pkg jobs already didn't build anything, and now they'd not only build something, but with a specific configuration?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ideally it would run on all platforms, but apparently task-cluster doesn't make that easy yet, from what sfink tells me. One platform is better than no platforms, however.

We could split up SM(pkg) into SM(pkg) and SM(pkg-build) and SM(pkg-test) or something, but none of the existing spidermonkey tasks are split up (they do builds and tests in the same task), so it was easier when adding the new SM(pkg) task to keep them lumped together in SM(pkg). Would it be valuable to split them up? If so, then we should file a new bug.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It seems unfair to pick on SM(pkg) for running on linux only when *none* of the SM(...) tests run anywhere but linux at the moment. Ideally they all would run on all platforms!

Comment 9

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

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #8)
> It seems unfair to pick on SM(pkg) for running on linux only when *none* of
> the SM(...) tests run anywhere but linux at the moment. Ideally they all
> would run on all platforms!

Taskcluster only runs on linux. So those are the ones that are currently labeled SM-tc(...), though I'm probably going to get rid of the -tc prefix because TC stuff is "real" now. However, there are SM(...) builds on Windows, and I'm working on adding some on osx. They're just run by buildbot, because TC support isn't there yet.

(In reply to Mike Hommey [:glandium] from comment #6)
> It's also bloating a job whose description has nothing to do with that. See,
> I thought those pkg jobs already didn't build anything, and now they'd not
> only build something, but with a specific configuration?

My intention is to make this job do both source and binary packaging. I don't expect the binary packages to be used by distributions, but I'd like them to be something you could download and link against (perhaps with a speculative patch made for a specific embedder's problem.) It's sort of an end-to-end packaging test. The better it matches a real live embedder's use case, the better. And if we're going to pick a favorite embedder, I'd say Servo would be a good choice.

It would be more pure to create a job that only generates a source package (and hence the platform and flags are irrelevant), and dependent jobs to do the rest. But that's more work, and at the moment will just add overhead. (Once OSX and Windows are on TC, I could see it being more of a win.)

Comment 10

[Mike Hommey [:glandium]]

Since I'm still working on bug 1176787, I can tell you this: there is no generic embedder need, and many of the options produce very different results, with different requirements. Example: need ctypes? that means you need NSPR, which adds a shared library to the picture, even if you didn't build for SM as a shared library. We /could/ build NSPR static on most platforms, but we still can't on Windows because Windows doesn't have what it takes to put NSPR in a static library (specifically, PRThread requires access to thread termination "notifications" you can only get from a DLL), and I'm not sure adding one more linkage difference between platforms is a good idea (there are so many differences already).Oh BTW, Windows always requires NSPR, contrary to other platforms, that only need it for ctypes.

(Also, related rant: most (all?) embedders would prefer --disable-jemalloc, but SM developers want --enable-jemalloc without having to set it manually...)

(Yes, after having spent so much time on bug 1176787, I'm bitter)

Comment 11

[Jim Blandy :jimb]

When we can do better --- when TC supports the other platforms; when bug 1176787 is fixed --- then we'll do better. I don't see any reason to stop doing the the degree of validation that is readily available to us for the time being.

Comment 12

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

(In reply to Mike Hommey [:glandium] from comment #10)
> Since I'm still working on bug 1176787, I can tell you this: there is no
> generic embedder need, and many of the options produce very different
> results, with different requirements. Example: need ctypes? that means you

I guess I'm not looking for one binary package that solves every embedder's need, but rather a single blessed path that produces something useful for a single embedder, preferably an embedder we care about. That alone should dramatically reduce the frequency of breaking all the other configurations of interest to embedders.

In addition, and maybe I'm just dreaming here, but I'd like to produce both opt + debug binaries that are useful to a decent percentage of embedders, mainly so (1) they won't whine as much when we demand that they link against a debug library before we'll help them; and (2) that the ones who are compiling their own configurations will use these as a baseline and try to make something at least as good for their own use.

> need NSPR, which adds a shared library to the picture, even if you didn't
> build for SM as a shared library. We /could/ build NSPR static on most
> platforms, but we still can't on Windows because Windows doesn't have what
> it takes to put NSPR in a static library (specifically, PRThread requires
> access to thread termination "notifications" you can only get from a DLL),
> and I'm not sure adding one more linkage difference between platforms is a
> good idea (there are so many differences already).Oh BTW, Windows always
> requires NSPR, contrary to other platforms, that only need it for ctypes.

We're trying to eliminate some of these configurations. For now, I'm personally fine with requiring a shared NSPR. If we get to the point where we can drop NSPR for interesting configurations, I'll do the work to eliminate NSPR from ctypes (by reverting to dumb #ifdefs that do the library loading and symbol lookup and stuff).

Other people have different opinions. A lot of people want to get rid of NSPR as quickly as possible. And Servo has its own constraints, which I don't know what are.

Right now, the browser build needs ctypes, so I'm more interested in the with-ctypes configuration than the no-NSPR configuration. (And besides, my own stuff that runs under the JS shell needs ctypes.)

> (Also, related rant: most (all?) embedders would prefer --disable-jemalloc,
> but SM developers want --enable-jemalloc without having to set it
> manually...)

To be argumentative, the JS shell tests are an important embedding, and they need jemalloc because Firefox uses jemalloc. So it's not *all* embedders. I'm curious about your statement, though, since I thought jemalloc was a substantial speed win. Do embedders prefer vanilla malloc, or do they have their own, or ?

> (Yes, after having spent so much time on bug 1176787, I'm bitter)

After rereading that bug, it seems you're well ahead of me on the requirements of the various embeddings. And I'm really happy that you are working on that, because it's way over my head and miserable work besides. We are extremely fortunate to have someone smart enough to be able to do that stuff, yet dumb^Wagreeable enough to sign up for and actually do the work. I don't like that it's such a pain for you, though.

What could we do on the spidermonkey side to make this easier? If we pushed through and eliminated NSPR on Windows and when using ctypes, would that make a significant difference?

I could also split apart this job to test out a bunch of different configurations under taskcluster, if it would help with bug 1176787. But so could you, and it wouldn't cover osx or Windows, so I'm doubtful it would help. Still, let me know if I could help out by doing something like that. (With some more effort, I can expand it to cover osx and windows via buildbot with modified builds.)

Comment 13

[Mike Hommey [:glandium]]

(In reply to Steve Fink [:sfink] [:s:] from comment #12)
> To be argumentative, the JS shell tests are an important embedding, and they
> need jemalloc because Firefox uses jemalloc. So it's not *all* embedders.
> I'm curious about your statement, though, since I thought jemalloc was a
> substantial speed win. Do embedders prefer vanilla malloc, or do they have
> their own, or ?

Think of it this way: when you link to a third party library, do you expect it to impose what allocator the whole process is going to use?

Examples of embedders that don't want jemalloc: servo, gnome shell, firefox.
Yes, in fact, even Firefox wants the equivalent of --disable-jemalloc from SM, because it has its own. But it doesn't actively do that because the build systems are intertwined (which participates in making things intractable), but it would if it were building SM like other embedders do.

(There are also funny details about having jemalloc enabled when embedding, but I'll leave them to the big comment I'll have to write for the patch in bug 1176787)

> What could we do on the spidermonkey side to make this easier? If we pushed
> through and eliminated NSPR on Windows and when using ctypes, would that
> make a significant difference?

One significant difference would come from removing the use of PRThread (and relying on the parts of NSPR that remain used not using it on its own). That would allow to link NSPR statically (not that it would change the number of combinations, there are other reasons to prefer keeping NSPR a shared library, but then they could be satisfied by building against system NSPR instead of building it in-tree).

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Mike Hommey [:glandium] from comment #13)
> Examples of embedders that don't want jemalloc: servo

Rust uses jemalloc by default IIRC, is the problem that its just a different copy?

> > What could we do on the spidermonkey side to make this easier? If we pushed
> > through and eliminated NSPR on Windows and when using ctypes, would that
> > make a significant difference?
> 
> One significant difference would come from removing the use of PRThread (and
> relying on the parts of NSPR that remain used not using it on its own).

Have you seen bug 956899? Terrence is chugging away at this as fast as he can :)

Comment 15

[Jack Moffitt [:jack]]

> Rust uses jemalloc by default IIRC, is the problem that its just a different
> copy?

Yes. We don't want to end up with two sets of jemalloc.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Mike Hommey [:glandium] from comment #13)
> One significant difference would come from removing the use of PRThread (and
> relying on the parts of NSPR that remain used not using it on its own). That
> would allow to link NSPR statically (not that it would change the number of
> combinations, there are other reasons to prefer keeping NSPR a shared
> library, but then they could be satisfied by building against system NSPR
> instead of building it in-tree).

Hey Mike! Bug 956899 and its dependents have landed and there is no more use of NSPR threading inside js/ anymore. What are the next steps for this bug and bug 1176787? Any updates? Anything else I can do to help?

Thanks!

Comment 17

[Mike Hommey [:glandium]]

Ctypes still requires nspr. While ctypes is not enabled by default when building standalone js, it is manually enabled in most standalone js jobs on automation. Do compaction, root analysis and hazard builds need ctypes enabled?

Comment 18

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

(In reply to Mike Hommey [:glandium] from comment #17)
> Ctypes still requires nspr. While ctypes is not enabled by default when
> building standalone js, it is manually enabled in most standalone js jobs on
> automation. Do compaction, root analysis and hazard builds need ctypes
> enabled?

Compaction and root analysis do not. Hazard builds do, both for building the shell that runs the analysis (since it relies on ctypes) and for finding hazards in ctypes code.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Steve Fink [:sfink] [:s:] from comment #18)
> (In reply to Mike Hommey [:glandium] from comment #17)
> Hazard builds do, both for building the
> shell that runs the analysis (since it relies on ctypes) and for finding
> hazards in ctypes code.

Hazard builds are the ones that build standalone js + the browser, right?

Comment 20

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

(In reply to Mike Hommey [:glandium] from comment #19)
> Hazard builds are the ones that build standalone js + the browser, right?

Yes.

If it would help, these builds could rely on a prebuilt js shell with ctypes, or depend on a building job to build it. But in the end, you need an opt js shell with ctypes, and then you need to build the whole browser (which includes ctypes).

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I don't think we need to do anything here outside of what bug 1277338 will do.