Fold js back into libxul

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: khuey, Assigned: glandium)

Tracking

Trunk
mozilla36
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(3 attachments, 4 obsolete attachments)

We should undo the hack from http://hg.mozilla.org/mozilla-central/rev/0ff917ab21e4 before final if at all possible.
I threw this at tryserver last night and it worked with no troubles.
http://hg.mozilla.org/mozilla-central/rev/53952ab4a544
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Backed out because this is triggering Bug 592960.

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=94bcf27e5948
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla2.0b8 → ---
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → ---
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Flags: in-testsuite-
Version: unspecified → Trunk
Depends on: 614186

Updated

8 years ago
blocking2.0: ? → -
FWIW this is still building on try (http://tbpl.mozilla.org/?tree=Try&rev=592abbf5069e). Can we try landing it again?
I'd like to know that it does not bring back bug 592960 first.
We should investigate whether the omission of the PGO input files for mozjs (which was corrected for FF5) was the cause of bug 592960.
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.
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.
Posted patch patch (obsolete) — Splinter Review
Attachment #535833 - Flags: review?(khuey)
Comment on attachment 535833 [details] [diff] [review]
patch

This looks ok ... we could always check it in and see what happens.
Attachment #535833 - Flags: review?(khuey) → review+
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.
Whiteboard: [Don't check in before figuring out JS PGO situation]
Posted patch patch v2 (obsolete) — Splinter Review
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.
Attachment #535833 - Attachment is obsolete: true
Attachment #544387 - Flags: review?(khuey)

Comment 14

8 years ago
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.
Posted patch patch v3 (obsolete) — Splinter Review
This will need to depend on whatever bug decides to explicitly turn JS PGO back on then, as Kyle pointed out in comment 12.
Attachment #544387 - Attachment is obsolete: true
Attachment #544392 - Flags: review?(khuey)
Attachment #544387 - Flags: review?(khuey)
Comment on attachment 544392 [details] [diff] [review]
patch v3

Yes I want incremental linking back!

r=me.
Attachment #544392 - Flags: review?(khuey) → review+
This one we can check in now.
(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.
(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.
I presume this can be landed now?
Which part?  We can land the incremental linking part, but I'd rather hold off on the rest due to comment 18.
Incremental linking *would* be nice...
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #25)
> Yeah, lets do it.

Can someone fold mozjs back this week?
(In reply to Taras Glek (:taras) from comment #26)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

Updated

8 years ago
Assignee: general → matjk7
Status: REOPENED → ASSIGNED
Depends on: 563318
Posted patch patch v3 (obsolete) — Splinter Review
The incremental linking stuff is now bug 696627.
Attachment #544392 - Attachment is obsolete: true
Attachment #568931 - Flags: review+
Depends on: 709193
Depends on: 750661
n.b.: bug 673518 was fixed along the way.
Whiteboard: [Don't check in before figuring out JS PGO situation]
(Assignee)

Updated

5 years ago
Assignee: matjk7 → mh+mozilla
(Assignee)

Updated

5 years ago
Attachment #568931 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8506075 - Flags: review?(gps) → review+
This needed a clobber touch for Windows builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d6b4070a93
Depends on: 914596
https://hg.mozilla.org/mozilla-central/rev/a758ca998666
https://hg.mozilla.org/mozilla-central/rev/c2d6b4070a93
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8507274 - Flags: review?(mshal) → review+
Attachment #8507278 - Flags: review?(mshal) → review+
(Assignee)

Comment 39

5 years ago
Backed out the removed-files.in part, as it appears it's not necessary anymore
https://hg.mozilla.org/integration/mozilla-inbound/rev/536c500f8e76
Are these changes going to make it harder for us to revert back to MSVC 2010 if we need to?
Reverting would require this to be backed out, yes.
Depends on: 1086539
Blocks: 1098595
You need to log in before you can comment on or make changes to this bug.