609976 - Fold js back into libxul

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should undo the hack from http://hg.mozilla.org/mozilla-central/rev/0ff917ab21e4 before final if at all possible.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I threw this at tryserver last night and it worked with no troubles.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/53952ab4a544

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out because this is triggering Bug 592960.

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=94bcf27e5948

Comment 4

[Serge Gautherie (:sgautherie)]

Ftr,

(In reply to comment #2)
> http://hg.mozilla.org/mozilla-central/rev/53952ab4a544

http://hg.mozilla.org/comm-central/rev/7996fcc11af9
Port 'Bug 609976: Fold js back into libxul on Windows now that PGO seems less grumpy.' to comm-central to try and fix trace malloc bustage on windows.

(In reply to comment #3)
> http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=94bcf27e5948

http://hg.mozilla.org/comm-central/rev/5f253d5c2fb7
Back out changeset 7996fcc11af9 / porting of bug 609976 due to test failures in mozilla-central and the mozilla-central changeset being backed out

Comment 5

[Matheus Kerschbaum]

FWIW this is still building on try (http://tbpl.mozilla.org/?tree=Try&rev=592abbf5069e). Can we try landing it again?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'd like to know that it does not bring back bug 592960 first.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We should investigate whether the omission of the PGO input files for mozjs (which was corrected for FF5) was the cause of bug 592960.

Comment 8

[Matheus Kerschbaum]

I tried pushing this to try last night (http://tbpl.mozilla.org/?tree=Try&rev=d973a8bf2e89) to get perf numbers and see if it regresses bug 592960, but to my surprise the build failed to compile at all (http://tinderbox.mozilla.org/showlog.cgi?log=Try/1306292468.1306312068.26873.gz).

Considering that this was working a few weeks ago, and that the size of xul.dll with statically linked JS is slightly over 17MB (in a local build using MSVC2010) now, I wonder if this is simply a hardcoded limit in MSVC2005 (ala bug 653662) rather than a compiler/linker bug.

If that's the case, this is yet another reason to switch to MSVC2010.

Comment 9

[Matheus Kerschbaum]

So it turns out the problem here is exactly the same as bug 653662. When we merge mozjs and libxul we go over the incremental linker limit and MSVC gives up compiling altogether.

Here's an all-green try-run of the patch in this bug with several JS reftest runs to make sure bug 592960 doesn't regress: http://tbpl.mozilla.org/?tree=Try&rev=6594e77dc7ba.

And compare-talos numbers: http://goo.gl/Np06A.

Comment 10

[Matheus Kerschbaum]

Attachment: patch

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 535833 [details] [diff] [review]
patch

This looks ok ... we could always check it in and see what happens.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Though, actually, this would turn PGO back on for JS, which we have explicitly turned off for the moment, so lets not check this in just yet.

Comment 13

[Matheus Kerschbaum]

Attachment: patch v2

This stopped working again for some reason. Since this is a likely a bug in MSVC 2005 I simply made the ENABLE_SHARED_JS hack check for the MSVC version as well. This patch also removes the PGO hack for JS (if that's ok) and re-enables incremental linking for debug builds using MSVC 2010.

Comment 14

[Robert Kaiser]

Comment on attachment 544387 [details] [diff] [review]
patch v2

Please keep turning on JS PGO in a separate bug and patch, it messes up all our crash stats, and that for some of the most high-profile crashes, we don't want that to be mixed into other changes that might have an impact on crash signatures.

Comment 15

[Matheus Kerschbaum]

Attachment: patch v3

This will need to depend on whatever bug decides to explicitly turn JS PGO back on then, as Kyle pointed out in comment 12.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 544392 [details] [diff] [review]
patch v3

Yes I want incremental linking back!

r=me.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This one we can check in now.

Comment 18

[Matheus Kerschbaum]

(In reply to comment #17)
> This one we can check in now.

That's probably not a good idea since it will mess with the results for bug 563319.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #18)
> (In reply to comment #17)
> > This one we can check in now.
> 
> That's probably not a good idea since it will mess with the results for bug
> 563319.

Mmm, fair point.

Comment 21

[(dormant account)]

I presume this can be landed now?

Comment 22

[Justin Lebar (not reading bugmail)]

khuey, ping re comment 21.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Which part?  We can land the incremental linking part, but I'd rather hold off on the rest due to comment 18.

Comment 24

[Justin Lebar (not reading bugmail)]

Incremental linking *would* be nice...

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, lets do it.

Comment 26

[(dormant account)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #25)
> Yeah, lets do it.

Can someone fold mozjs back this week?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Taras Glek (:taras) from comment #26)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #25)
> > Yeah, lets do it.
> 
> Can someone fold mozjs back this week?

'it' is turning incremental linking back out.  We're not going to fold mozjs back in until we switch compilers.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

s/out/on

Comment 29

[Matheus Kerschbaum]

Attachment: patch v3

The incremental linking stuff is now bug 696627.

Comment 30

[(not currently active) Ted Mielczarek]

n.b.: bug 673518 was fixed along the way.

Comment 31

[Mike Hommey [:glandium]]

Attachment: Fold mozjs.dll back into xul.dll

See bug 922912 comment 16.

Comment 32

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a758ca998666

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This needed a clobber touch for Windows builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d6b4070a93

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a758ca998666
https://hg.mozilla.org/mozilla-central/rev/c2d6b4070a93

Comment 35

[Mike Hommey [:glandium]]

Attachment: Followup to - Build ICU as a shared library on windows

Comment 36

[Mike Hommey [:glandium]]

Attachment: Update removed-files.in to account for the removal of mozjs.dll

Comment 37

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/887c54a2dc05
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fb6b5bc62a

Comment 38

[Mike Hommey [:glandium]]

Followup to unbust windows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe9f4f91599

Comment 39

[Mike Hommey [:glandium]]

Backed out the removed-files.in part, as it appears it's not necessary anymore
https://hg.mozilla.org/integration/mozilla-inbound/rev/536c500f8e76

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/887c54a2dc05
https://hg.mozilla.org/mozilla-central/rev/cbe9f4f91599

Comment 41

[(not currently active) Ted Mielczarek]

Are these changes going to make it harder for us to revert back to MSVC 2010 if we need to?

Comment 42

[Benjamin Smedberg]

Reverting would require this to be backed out, yes.