Closed Bug 1432340 Opened 2 years ago Closed 2 years ago

Introduce vm/FreeOp.h to contain that class, extricating it from the far-more-ponderous vm/Runtime.h for those users needing *only* js::FreeOp and nothing else

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(1 file)

Moving this out this way, means subsequent split-up Intl files can include just this file and avoid picking up vm/Runtime.h.

I used vm/FreeOp.h, but as FreeOp is mostly involved in finalizing and sweeping, at a quick skim of FreeOp uses, maybe it should be gc/FreeOp.h.  Feel free to ask for that move if desired.
Attached patch PatchSplinter Review
Attachment #8944586 - Flags: review?(sphink)
Blocks: 1431957
Comment on attachment 8944586 [details] [diff] [review]
Patch

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

Looking through the output of

    perl -lne '$f = $1 if /^([^\s{#].*)/; print $f if /fop->/' **/*.cpp

and piping it through

    perl -lne 'print $1 if /(\w+)\(/' | sort | uniq -c

it looks like the only non-obviously finalization functions that use FreeOp are

queueForBackgroundSweep
queueForForegroundSweep
sweep
sweepAll
sweepZoneGroups

decrementStepModeCount
FinishDiscardBaselineScript
FinishInvalidation
FreeJobQueueHandling
Invalidate
recompile
ReleaseAllJITCode
releaseData
ReleaseScriptCounts
setNewStepMode
unlinkDependentWasmImports
unlinkFromRuntime

So there's a little bit of stuff there that might not be directly sweep or finalization related, but I guess it feels GC-centric enough that I'm going to vote for gc/FreeOp.h. Even though this is not GC memory so it doesn't relate to gc/Allocator.h at all. But it does correspond to gc/Memory.h, which is all about the interface between the GC and the core memory allocation routines, and similarly this seems to be almost entirely about the interface between the GC and malloc/free.

It's still a little unsatisfying, but I'm swayed by the argument that if someone is looking for how to allocate and free some (non-GC) heap memory from the spidermonkey VM, I'd rather they not stumble across vm/FreeOp.h and get confused by it.

Sorry for the churn.
Attachment #8944586 - Flags: review?(sphink) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60e1a827ac56
Introduce gc/FreeOp.h to contain that class, extricating it from the far-more-ponderous vm/Runtime.h for those users needing *only* js::FreeOp and nothing else.  r=sfink
https://hg.mozilla.org/mozilla-central/rev/60e1a827ac56
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.