Closed Bug 883466 Opened 6 years ago Closed 6 years ago

Fix the GGC build failures caused by the landing of bug 880565

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
I'm not sure if this is the best strategy, but it works for my local build. Yes, I do realize that's how we got into this mess in the first place. Better solutions more than welcome.
Attachment #763017 - Flags: review?(n.nethercote)
Comment on attachment 763017 [details] [diff] [review]
v0

Looks like I spoke too soon. It fixes the compilation error, but linking fails. Will look into it more on Monday morning.
Attachment #763017 - Attachment is obsolete: true
Attachment #763017 - Flags: review?(n.nethercote)
Comment on attachment 763017 [details] [diff] [review]
v0

Review of attachment 763017 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, I think inlines.h files are a menace and I'm all in favour of code being moved out of them into either .h or .cpp files (depending on the code's hotness) wherever possible.
Attached patch v1Splinter Review
gc/Nursery.cpp depends on the following 3 inline methods with out-of-line definitions.

  - NewObjectCache::clearNurseryObjects
  - Allocator::reportAllocationOverflow
  - Allocator::onOutOfMemory

These used to be exposed through jsobjinlines.h, but aren't anymore. I've made these methods fully out-of-line: if any of these are hot, we have more serious problems.

A harder case is CanBeFinalizedInBackground (CBFIB). The use of CBFIB in gc/Nursery.cpp is not hot, but other users live right on the allocation path, so are hot by default. CBFIB depends on IsBackgroundFinalized in jsgc.h and Class in jsclass.h, so I moved it to jsgc.h from jsobjinlines.h. We have to add jsclass.h to jsgc.h's imports, but it only transitively pulls in things which are already transitively pulled into jsgc.h already.
Attachment #763750 - Flags: review?(n.nethercote)
Blocks: 880041
And the is<T> patches broke us too.
Attachment #763770 - Flags: review?(n.nethercote)
Comment on attachment 763750 [details] [diff] [review]
v1

Review of attachment 763750 [details] [diff] [review]:
-----------------------------------------------------------------

The less code we have in inlines.h files, the better!
Attachment #763750 - Flags: review?(n.nethercote) → review+
Attachment #763770 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/20ef0590dd57
https://hg.mozilla.org/mozilla-central/rev/fab9f7a443ad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.