Closed
Bug 1259850
Opened 8 years ago
Closed 8 years ago
Clean up the hazard analysis implementation
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
(Blocks 1 open bug)
Details
Attachments
(23 files, 4 obsolete files)
8.26 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
10.52 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
1.46 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
12.31 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
3.42 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
8.15 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
5.95 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
18.22 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
7.32 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
4.91 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
29.67 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
7.00 KB,
patch
|
Details | Diff | Splinter Review | |
9.61 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
5.56 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
3.98 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
10.27 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
61.52 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
2.26 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Making a bug for a large grab-bag of changes I've been working on. Some are just refactoring. Some are actual fixes for things uncovered while creating a test suite. Then there's the test suite itself.
Assignee | ||
Comment 1•8 years ago
|
||
It seems like Map is now the "right" solution for... well, maps. Dicts. Whatever you want to call them. This moves over some but not all of them. I think this was motivated by a case where there was a C function named... uh, "push"? Hm, I doubt I would be doing x.push(). But it was something like that -- it broke the analysis because it shadowed something on Object.prototype that I was unwisely calling. Clearly, it wasn't a *real* problem, since the current code is not failing. But this patch adds a bunch of comments too.
Attachment #8734955 -
Flags: review?(terrence)
Assignee | ||
Comment 2•8 years ago
|
||
Honestly, I'm not sure why I'm wasting your time with this one. I should've rolled it into my other refactoring patch.
Attachment #8734956 -
Flags: review?(terrence)
Assignee | ||
Comment 3•8 years ago
|
||
I think I'm the only one who ever looks at them, but in hazard reports it gives you a patch from a point early in a variable's live range, through a GC call, to a use of the variable after the GC call that proves it's still live. It can be very helpful in deciphering tricky hazards without having to go back to the XDB files. But it was sometimes cutting off the first link in that chain, which made it seem like it was just spewing a bunch of internal goop. Perhaps with this fixed, other people will get some use out of it too? I meant to write a test for this, but ran out of steam. I guess I'll wait until I find the next problem. It's noncritical stuff anyway. Oh, crud. I just noticed this patch has a bunch of other stuff mixed in.
Attachment #8734961 -
Flags: review?(terrence)
Assignee | ||
Comment 4•8 years ago
|
||
Another trivial one, from trying to split out the important stuff a little better.
Attachment #8734967 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734961 -
Attachment is obsolete: true
Attachment #8734961 -
Flags: review?(terrence)
Assignee | ||
Comment 5•8 years ago
|
||
Larger chunk of refactoring goop, including a somewhat controversial Map.prototype.update definition.
Attachment #8734970 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734967 -
Attachment is obsolete: true
Attachment #8734967 -
Flags: review?(terrence)
Assignee | ||
Comment 6•8 years ago
|
||
Previously, this mostly "worked" purely by chance -- it started gathering method definitions at the 'csu' variable, which JS helpfully hoisted up to the toplevel. It had the last value assigned to csu within the loop, which would have been the basest base class (don't think too hard about the case of multiple inheritance, it was wrong). Then we find all descendants, which was *too* much, but it ended up just making the analysis conservative. The new version gets the exact set of virtual methods that are possible, starting at the static type. (So all descendants' definitions, plus the static type itself or anything it might have inherited from.)
Attachment #8734991 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734970 -
Attachment is obsolete: true
Attachment #8734970 -
Flags: review?(terrence)
Assignee | ||
Comment 7•8 years ago
|
||
No more passing a 2nd return value back through a mutable 1-element array. Some of the comments were outright lies.
Attachment #8734997 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734991 -
Attachment is obsolete: true
Attachment #8734991 -
Flags: review?(terrence)
Assignee | ||
Comment 8•8 years ago
|
||
Seeing the exact commands executed (together with necessary environment variable changes) is really handy for debugging, but it also obscures what's going on.
Attachment #8734998 -
Flags: review?(terrence)
Assignee | ||
Comment 9•8 years ago
|
||
Whoa, math is hard and tests are good. So I have w workers that want to divide n tasks between themselves. While that means it'll be *about* Math.floor(n/w) tasks each, the specifics are important. With my previous logic, you'd drop n % w jobs on the floor. Which is not good. A different approach would be to give each worker Math.floor(n/w) tasks, and then give one of the remaining n%w tasks to each of the workers in order. That gets a nice even distribution, but it seemed a little messy. I guess that would be num_tasks[i] = Math.floor(n/w) + (i < n%w ? 1 : 0), which isn't bad. Hm, maybe I should have done that. Although I really care about finding the start and end points, which means iterating through all previous workers' (the workers figure this out on startup). I guess that's still fixable without a loop, with some logic. Anyway, what I ended up with was finding the w (floating point) points at which to break up the n tasks. Then each worker's tasks go from its start point (floored to the previous whole index) up to just before the beginning of the next worker's task start point. (If start < end, then that worker gets no tasks.) The n%w "extra" tasks get evenly spread across workers this way, and each worker just needs to compute its start position and the next worker's start position. (There is a virtual worker that starts at the task just past the end, assuming weird rounding that would break Math.floor(k/k*N)==N, so the final worker need not be handled specially.)
Attachment #8734999 -
Flags: review?(terrence)
Assignee | ||
Comment 10•8 years ago
|
||
I switched lots of stuff over to in-source annotations, but things like AutoSuppressGC were still done with an explicit list in annotations.js. This switches them over to in-source annotations too. That requires reordering the analysis phases, since we need to know the GC suppressors while building the callgraph. That may seem weird, but edges in the callgraph include information about whether the call is within the scope of a GC suppression.
Attachment #8735001 -
Flags: review?(terrence)
Assignee | ||
Comment 11•8 years ago
|
||
A pretty small grab-bag of minor fixes.
Attachment #8735002 -
Flags: review?(terrence)
Assignee | ||
Comment 12•8 years ago
|
||
I knew the compiler sometimes generated "C4" constructors and then internally made all of the regular constructors call them. Now, it's doing the same thing with "D4" destructors. In pretty common cases, too. I have a test that hits this in a later patch. This fixes a real problem that bz noticed. I tried removing some annotations that should have revealed a hazard, and it did not report one because it couldn't see that a destructor was GC'ing.
Attachment #8735003 -
Flags: review?(terrence)
Assignee | ||
Comment 13•8 years ago
|
||
The analysis attempts to give example stacks demonstrating how a function could GC, and some of my hazards were missing them. It turned out to be because of a wacky case where the mangled representation of a function was identical between two different functions -- gcFunctions.txt only had a record of the "real" one. Fortunately, it's pretty easy to fix, as I was already collecting the set of unmangled names for each mangled name, and they *do* differ in the unmangled name.
Attachment #8735004 -
Flags: review?(terrence)
Assignee | ||
Comment 14•8 years ago
|
||
Rewrite the test suite, and then test a bunch of stuff that this patch stack deals with, along with some "vanilla" sorts of hazards.
Attachment #8735005 -
Flags: review?(terrence)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8734961 [details] [diff] [review] The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further Uh oh. bzexport is flaking out on me and obsoleting things that should not be obsoleted. Reinstating review requests. On the bright side, this lets me flood terrence's inbox even more!
Attachment #8734961 -
Attachment is obsolete: false
Attachment #8734961 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734967 -
Attachment is obsolete: false
Attachment #8734967 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734970 -
Attachment is obsolete: false
Attachment #8734970 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8734991 -
Attachment is obsolete: false
Attachment #8734991 -
Flags: review?(terrence)
Comment 16•8 years ago
|
||
Comment on attachment 8734955 [details] [diff] [review] Switch to using a Map for visited points within a function, and heavily comment Review of attachment 8734955 [details] [diff] [review]: ----------------------------------------------------------------- Nice improvement!
Attachment #8734955 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8734956 -
Flags: review?(terrence) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8734961 [details] [diff] [review] The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further Review of attachment 8734961 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeRoots.js @@ +630,5 @@ > loadPrintedLines(functionName); > > while (entry) { > var ppoint = entry.ppoint; > + var lineText = findLocation(entry.body, ppoint, "brief"); Oh wow. That's just special. "brief" is "true", but I don't know that this abuse of the type system is called for. How about we pass {brief:true} and have findLocation take options={brief:false} instead?
Attachment #8734961 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8734967 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8734970 -
Flags: review?(terrence) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8734991 [details] [diff] [review] Start searching from the most specialized csu Review of attachment 8734991 [details] [diff] [review]: ----------------------------------------------------------------- Wow.
Attachment #8734991 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8734997 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8734998 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8734999 -
Flags: review?(terrence) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8735001 [details] [diff] [review] In-source annotations for GC suppression Review of attachment 8735001 [details] [diff] [review]: ----------------------------------------------------------------- Neat!
Attachment #8735001 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8735002 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8735003 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8735004 -
Flags: review?(terrence) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8735005 [details] [diff] [review] Rewrite the test suite, add several tests Review of attachment 8735005 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/t/testlib.py @@ +37,5 @@ > + print("Running %s" % cmd) > + subprocess.check_call(["sh", "-c", cmd]) > + > + def load_db_entry(self, dbname, pattern): > + if not isinstance(pattern, basestring): Please add a comment that pattern must be str or regexp. @@ +40,5 @@ > + def load_db_entry(self, dbname, pattern): > + if not isinstance(pattern, basestring): > + output = subprocess.check_output([self.binpath("xdbkeys"), dbname + ".xdb"]) > + entries = output.splitlines() > + matches = [f for f in entries if re.search(pattern, f)] s/f/_/. Underscore is the expected throwaway in these cases. @@ +57,5 @@ > +sixgill_bin = '{bindir}' > +'''.format(scriptdir=scriptdir, bindir=self.cfg.sixgill_bin)) > + cmd = [ os.path.join(scriptdir, "analyze.py"), phase ] > + if upto: > + cmd += [ "--upto", upto ] Strip extra space after and before [ and ] respectively; here and 2 lines up. @@ +72,5 @@ > + > + def computeHazards(self): > + self.run_analysis_script("callgraph") > + > + def load_text_file(self, filename, key=lambda l: l): It looks like we're using |key| for filtering. In python, a functor key usually indicates a sort key rather than a filter function. How about we call it |filter_func| instead? @@ +75,5 @@ > + > + def load_text_file(self, filename, key=lambda l: l): > + return [processed for processed in ( > + key(line.strip()) for line in file(os.path.join(self.outdir, filename)) > + ) if processed is not None] Ouch, that spacing. How about we name the intermediate generator object instead: keyed = (key(line.strip()) for line in file(os.path.join(self.outdir, filename))) return [_ for _ in keyed if _ is not None] @@ +84,5 @@ > + def load_gcTypes(self): > + def grab_type(line): > + m = re.match(r'^(GC\w+): (.*)', line) > + if m: > + return (m.group(1) + 's', m.group(2)) Please explicitly |return None| in the failing case here so that readers don't think it might be accidental. @@ +101,5 @@ > + if m: > + info = list(m.groups()) > + info[0] = info[0].split("$")[-1] > + info[3] = info[3].split("$")[-1] > + return HazardSummary(*info) And here.
Attachment #8735005 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 21•8 years ago
|
||
This was a fun one to track down. The analysis thought the *constructor* of AutoFoo here can GC: class GCOnDestruction { public: ~GCOnDestruction() { GC(); } }; extern void foo(); class AutoFoo { GCOnDestruction killmenow; public: AutoFoo() : killmenow() { foo(); } }; Why? Because if exceptions were allowed, then theoretically foo() could throw an exception, and the half-constructed AutoFoo would get immediately torn down, running killmenow's destructor. Which is fine and all, except that we're compiling with -fno-exceptions. For whatever reason, the CFG passed to the plugin contains exception handling cleanup nodes, and the analysis took that very seriously. Fortunately, it also marks those cleanup nodes as being only valid for exception handling, so we can manually ignore them. (Apparently, it uses cleanup nodes for other sorts of failure as well, and we do need to take those into account.)
Attachment #8746783 -
Flags: review?(terrence)
Assignee | ||
Comment 22•8 years ago
|
||
This was ugly. js::ReportOutOfMemory(ExclusiveContext*) was getting called, and yet it had no callees. Why? Because only js::ReportOutOfMemory(js::ExclusiveContext*) had any callees (notice the js:: namespace qualification). It actually got it right most of the time, but there was one recently-added usage with a forward declaration of ExclusiveContext was tricking it, and when it did a lookup from the mangled name => the demangled name, it happened to pick the unqualified version and everything fell apart. The fix appeared to be easy -- just use decl_as_string instead of pulling out the internal char* pointer. Except that decl_as_string is C++ only, so we get a link failure against cc1 (eg when compiling conftest.c). Turns out sixgill already had something set up for this, where it defines a bunch of dummy symbols that just abort() when they're called. They're weak, so they get overridden by the real ones, and the code dynamically chooses to only call them in the configuration where they exist. I changed decl_as_string to do the right thing when called for C code instead, and can now call decl_as_string unconditionally for either language.
Attachment #8746787 -
Flags: review?(terrence)
Assignee | ||
Comment 23•8 years ago
|
||
This is pointing to the binary resulting from the sixgill patches uploaded to this bug. No need to review this separately (and in fact I should prune down the diff as there are nuisance whitespace changes in it now... ugh). MozReview-Commit-ID: 7WSYmLAVb7M
Assignee | ||
Comment 24•8 years ago
|
||
So... an earlier patch in this bug fixed a breakage in the callgraph related to internally generated constructors and destructors. Unfortunately, that made all pre barriers into GC functions. With a very plausible-looking stack. :( Fortunately, it turns out to be a false positive, The GC happened in AutoClearTypeInferenceStateOnOOM, which is passed in to various sweep() functions by pointer. In most cases, this pointer is actually null, and so the GC is impossible. I split out separate function overloads to say whether it was really possible. They actually just feed into the original function, but the nogc variant suppresses the analysis. (If I got it wrong, it'll be a dynamic MOZ_CRASH.) The details are definitely arguable. I used the same name for all 3 methods, which perhaps isn't the best choice. Lemme know what you think. MozReview-Commit-ID: CLrEs3OODOu
Attachment #8746795 -
Flags: review?(terrence)
Assignee | ||
Comment 25•8 years ago
|
||
The code was previously doing some very very heuristic-y checks for identifying constructors. This broke when sixgill started properly qualifying type names, so I did the verbose ugly thing that is much much more precise. It still does some nasty regexp parsing, and it's possible to fool, but very unlikely. MozReview-Commit-ID: 46064amRbCQ
Attachment #8746796 -
Flags: review?(terrence)
Assignee | ||
Comment 26•8 years ago
|
||
Here's an easy one. Get rid of some unused test files. MozReview-Commit-ID: sBZct85zDJ
Attachment #8746798 -
Flags: review?(terrence)
Assignee | ||
Comment 27•8 years ago
|
||
Add a test for the new fixes. MozReview-Commit-ID: H5NZlyS2rx5
Attachment #8746799 -
Flags: review?(terrence)
Updated•8 years ago
|
Attachment #8746783 -
Flags: review?(terrence) → review+
Comment 28•8 years ago
|
||
Comment on attachment 8746787 [details] [diff] [review] Use decl_as_string to properly namespace-qualify types for C++ Review of attachment 8746787 [details] [diff] [review]: ----------------------------------------------------------------- O_o
Attachment #8746787 -
Flags: review?(terrence) → review+
Comment 29•8 years ago
|
||
Comment on attachment 8746795 [details] [diff] [review] Expose when sweeping can or cannot GC to the analysis Review of attachment 8746795 [details] [diff] [review]: ----------------------------------------------------------------- I think this is clearer to the human reader too. ::: js/src/jsscript.h @@ +1725,5 @@ > > inline js::TypeScript* types(); > > + void maybeSweepTypes(js::AutoClearTypeInferenceStateOnOOM& oom) { > + maybeSweepTypes(&oom); This is a definite double-take. We should call the target maybeSweepTypesInner or somesuch to disambiguate. ::: js/src/vm/ObjectGroup.h @@ +409,1 @@ > inline void maybeSweep(AutoClearTypeInferenceStateOnOOM* oom); And here as well, please.
Attachment #8746795 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8746796 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8746798 -
Flags: review?(terrence) → review+
Comment 30•8 years ago
|
||
Comment on attachment 8746799 [details] [diff] [review] Test for sixgill exception handling + analysis constructor/destructor matching Review of attachment 8746799 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/t/exceptions/source.cpp @@ +22,5 @@ > +class AutoSomething { > + RAII_GC gc; > + public: > + AutoSomething() : gc() { > + asm(""); // Ooh, scary, this might throw an exception :-)
Attachment #8746799 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 31•8 years ago
|
||
Well, this got a little out of hand. I wanted to create a new ZoneCellIter that knew statically that it would not GC during its constructor. I can't make a templateized ZoneCellIter<T>, because you can't have both templatized and non-templatized classes with the same name. So I used ZoneNonNurseryCellIter<T>, where T is an AllocKind, but it's fugly. I also noticed that a lot of these did some wonky JSScript* script = i.get<JSScript>(); stuff, which felt sort of redundant with ZoneNonNurseryCellIter<AllocKind::SCRIPT>. And then I thought I'd be clever and use MapTypeToFinalizeKind. Especially since then maybe I could use SFINAE to make it work for all GC types. But it still had the ugly type name. So I thought maybe it should be a method on Zone anyway. But Zone doesn't include jsgcinlines.h, so it doesn't have the types. Ooh, but this is template-land, so it doesn't need them. So I switched stuff over to eg zone->cellIter<JSScript>(), it looked kinda cool. And I realized that zone->cellIter<JSObject>() is kind of pointless, since JSObject does not correspond to a single AllocKind anyway. That meant I could leave it as a zone->cellIter(kind) overload. And life was good. Except now ZoneCellIterUnderGC stuck out like a sore thumb. So I switched it over to a similar setup. And then I thought that if I'm changing the way iteration happens, I may as well go all the way and support new-style C++11 iteration. And then I looked at the time. And said "no". MozReview-Commit-ID: 77GjtXY0QUf
Attachment #8749460 -
Flags: review?(terrence)
Comment 32•8 years ago
|
||
Comment on attachment 8749460 [details] [diff] [review] Make ZoneNonNurseryCellIter to communicate nogc to the hazard analysis Review of attachment 8749460 [details] [diff] [review]: ----------------------------------------------------------------- At first I was bewildered by the many new iter types, but |auto| makes the usage sites quite clean while giving us the type flexibility to cleanly represent the annoying plurality of potential states! Quite nice. I'd r+ this except that I want Jon to take a look too, in case he sees something I missed.
Attachment #8749460 -
Flags: review?(terrence) → review?(jcoppeard)
Assignee | ||
Comment 33•8 years ago
|
||
Actually, Terrence was totally wrote about the explosion of ZoneCellIter types. I mapped out what each one was doing, and I *think* I got all the variants properly contained within two types: ZoneCellUntypedIter and ZoneCellIter<T> (where T is a GC type, not an AllocKind). I am submitting this as a separate patch because I think it may be easier to follow the logic of introducing the ZoneNonNurseryCellIter nonsense first, and then reducing it down. But I'm not sure. A squashed patch might actually be simpler to review. MozReview-Commit-ID: AxRTTUfWrND
Attachment #8749896 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 34•8 years ago
|
||
err... "totally *right*".
Assignee | ||
Comment 35•8 years ago
|
||
Also create accessors on gc::Zone for a nicer API to ZoneCellIters. Here's a version that actually compiles and stuff. It also forces you to pass through a token to discardJitCode that proves that you've emptied the nursery somehow. It does not eliminate any hazards. MozReview-Commit-ID: AxRTTUfWrND
Attachment #8751115 -
Flags: review?(jcoppeard)
Comment 36•8 years ago
|
||
Comment on attachment 8751115 [details] [diff] [review] Make ZoneCellIter variants to communicate nogc to the hazard analysis Review of attachment 8751115 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for posting the combined patch. It's a nice cleanup for zone iteration. ::: js/src/gc/Iteration.cpp @@ +103,4 @@ > > if (compartment) { > + Zone* zone = compartment->zone(); > + for (auto script = zone->cellIter<JSScript>(underGC); !script.done(); script.next()) { This is not called under GC as far as I can see - it's used for Debugger::findScripts. Can we just call the normal iterator? @@ +125,3 @@ > > for (auto thingKind : ObjectAllocKinds()) { > + for (auto obj = zone->cellIter<JSObject>(thingKind, underGC); !obj.done(); obj.next()) { It looks like this is called by the CC so that would not be under GC either. Maybe just call the normal iterator. ::: js/src/gc/Zone.h @@ +159,5 @@ > + // Iterate over all cells in the zone. See the definition of ZoneCellIter > + // in jsgcinlines.h for the possible arguments and documentation. > + template <typename T, typename... Args> > + js::gc::ZoneCellIter<T> cellIter(Args... args) { > + return js::gc::ZoneCellIter<T>(this, args...); I think we should mozilla::Forward the args here, and below. @@ +172,5 @@ > + // dependent on the type T, so does not need any special handling.) > + template <typename... Args> > + js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...> untypedCellIter(Args... args) { > + return js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...>(this, args...); > + } Do we need this for anything other than objects? If not could we just have an objectIter() that returns ZoneCellIter<JSObject*>? ::: js/src/jsgc.cpp @@ +2440,5 @@ > AutoClearTypeInferenceStateOnOOM oom(zone); > + AutoAssertUnderGC underGC(rt); > + > + for (auto obj = zone->cellIter<JSObject>(AllocKind::OBJECT0, underGC); !obj.done(); obj.next()) > + obj->isNative(); Not sure this is meant to be here. ::: js/src/jsgcinlines.h @@ +190,5 @@ > + JSRuntime* rt; > + > + public: > + AutoAssertUnderGC(JSRuntime* rt) : AutoAssertEmptyNursery(rt) { > + MOZ_ASSERT(rt->isHeapBusy()); This doesn't check we're under GC as it also allows for heapState_ to be Tracing. I think this should be |rt->isHeapCollecting()|. Ditto below. @@ +284,2 @@ > public: > + ZoneCellUntypedIter_Dependent(Args... args) : ZoneCellUntypedIter(args...) {} Needs Forward if we keep this.
Attachment #8751115 -
Flags: review?(jcoppeard) → review+
Updated•8 years ago
|
Attachment #8749460 -
Attachment is obsolete: true
Attachment #8749460 -
Flags: review?(jcoppeard)
Updated•8 years ago
|
Attachment #8749896 -
Attachment is obsolete: true
Attachment #8749896 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 37•8 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #36) > ::: js/src/gc/Iteration.cpp > @@ +103,4 @@ > > > > if (compartment) { > > + Zone* zone = compartment->zone(); > > + for (auto script = zone->cellIter<JSScript>(underGC); !script.done(); script.next()) { > > This is not called under GC as far as I can see - it's used for > Debugger::findScripts. Can we just call the normal iterator? Oh. Yeah, it turns out that the ZoneCellIterUnderGC was really ZoneCellIterWhileHeapIsBusy. So at the moment, zone->cellIter<JSScript>() is just as good, since it'll skip the evictNursery. The busy-heap version doesn't seem very useful, since afaict the only effect is to skip a dynamic if (!rt->isHeapBusy) check? > ::: js/src/gc/Zone.h > @@ +159,5 @@ > > + // Iterate over all cells in the zone. See the definition of ZoneCellIter > > + // in jsgcinlines.h for the possible arguments and documentation. > > + template <typename T, typename... Args> > > + js::gc::ZoneCellIter<T> cellIter(Args... args) { > > + return js::gc::ZoneCellIter<T>(this, args...); > > I think we should mozilla::Forward the args here, and below. Oh, yeah. > @@ +172,5 @@ > > + // dependent on the type T, so does not need any special handling.) > > + template <typename... Args> > > + js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...> untypedCellIter(Args... args) { > > + return js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...>(this, args...); > > + } > > Do we need this for anything other than objects? If not could we just have > an objectIter() that returns ZoneCellIter<JSObject*>? I eventually realized that I could eliminate not only this, but also ZoneCellUntypedIter, by using ZoneCellIter<TenuredCell> as a base class and specializing it to be pretty much what ZoneCellUntypedIter was. Then I don't need to play any special tricks to get Zone.h to compile without the type definitions, and it should continue working properly as we make more stuff nursery-allocable (including things with multiple AllocKinds, if we were to create such a thing.) If you want to iterate over cells of arbitrary type (eg because you're looping over all AllocKinds), then you can just use ZoneCellIter<TenuredCell>. > ::: js/src/jsgc.cpp > @@ +2440,5 @@ > > AutoClearTypeInferenceStateOnOOM oom(zone); > > + AutoAssertUnderGC underGC(rt); > > + > > + for (auto obj = zone->cellIter<JSObject>(AllocKind::OBJECT0, underGC); !obj.done(); obj.next()) > > + obj->isNative(); > > Not sure this is meant to be here. Oops, sorry. That's debugging code because it turns out there weren't any instances of iterating over objects, and my first cut didn't compile. So this was to force an instantiation. Gone now. > ::: js/src/jsgcinlines.h > @@ +190,5 @@ > > + JSRuntime* rt; > > + > > + public: > > + AutoAssertUnderGC(JSRuntime* rt) : AutoAssertEmptyNursery(rt) { > > + MOZ_ASSERT(rt->isHeapBusy()); > > This doesn't check we're under GC as it also allows for heapState_ to be > Tracing. I think this should be |rt->isHeapCollecting()|. Ditto below. Yeah, I've eliminated AutoAssertUnderGC in favor of AutoAssertHeapBusy and AutoAssertEmptyNursery. The latter is what I actually care about with respect to hazards. I'm not sure if the former is useful? Currently, I don't pay attention to it, as it would only allow skipping what looks to be a cheap dynamic check. But I may be missing something. I can always add overloads that take it if there's something useful to be done with it; I just didn't see the need for the added complexity right now.
Attachment #8752306 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•8 years ago
|
Attachment #8751115 -
Attachment is obsolete: true
Assignee | ||
Comment 38•8 years ago
|
||
Oops. Didn't wait for the compile to finish. What could I possibly break by adding mozilla::Forward<>? Heh. It triggered a *nasty* template compilation error message, which ended up being due to bad const hygiene. This version compiles for me. (Hopefully it will on Windows too.)
Attachment #8752314 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•8 years ago
|
Attachment #8752306 -
Attachment is obsolete: true
Attachment #8752306 -
Flags: review?(jcoppeard)
Comment 39•8 years ago
|
||
Comment on attachment 8752314 [details] [diff] [review] Make ZoneCellIter variants to communicate nogc to the hazard analysis Review of attachment 8752314 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. ::: js/src/gc/Iteration.cpp @@ +124,2 @@ > if (obj->asTenured().isMarked(GRAY)) > + cellCallback(data, JS::GCCellPtr(static_cast<JSObject*>(obj))); Can we call the get() method here and get rid of the cast? ::: js/src/jsgc.h @@ +1352,5 @@ > + AutoAssertHeapBusy() : rt(nullptr) { > + } > + > + public: > + AutoAssertHeapBusy(JSRuntime* rt) { This is going to warn about implicit conversion from JSRuntime* to AutoAssertHeapBusy. You probably want to mark this |explicit|. Ditto for AutoAssertEmptyNursery. ::: js/src/jsgcinlines.h @@ +297,3 @@ > public: > + // (Possibly) outside of a GC, non-nursery allocated > + template <typename = MapTypeToFinalizeKind<GCType>> Wow, I've never see this syntax before! It seems the template parameter name is optional... More importantly, is this necessary? I tried removing this line and the one below and the code still compiles. ::: js/src/vm/TypeInference.cpp @@ +4438,2 @@ > zone->setPreservingCode(false); > + zone->discardJitCode(rt->defaultFreeOp(), rt); This relies on implicit conversion from JSRuntime* to AutoAssertEmpty nursery. I think this should just pass |empty| here.
Attachment #8752314 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 40•8 years ago
|
||
A pair of recently introduced hazards are only visible with the fixes in this bug. Annotate them.
Attachment #8756633 -
Flags: review?(terrence)
Updated•8 years ago
|
Attachment #8756633 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 41•8 years ago
|
||
Oh boy. Landing based on https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f39ef038eb6c6dc968c1e12114261ef9eade0c8
Comment 42•8 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/15de0d1d0b05 Switch to using a Map for visited points within a function, and heavily comment, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/e8544b072ee6 Rename function, add a few comments, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d2730ada95 The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/0d58f8529b86 Convert memoizer to Map, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/b6108a65dc38 Refactoring: more Set/Map usage, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/196ac1f813f9 Start searching from the most specialized csu, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/c6615c7b0083 Refactor findVirtualFunctions and improve comments, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/266befcb8acd Hide command output behind --verbose flag, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/d00b9c8a7984 Fix chunking implementation, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/129559d4ac62 In-source annotations for GC suppression, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/5762a8fba027 Various refactorings, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/457cb29cad55 Handle D4 destructors, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7373c6c29e Provide GC stacks for synthesized functions, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/aa434447a11b Rewrite the test suite, add several tests, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/b641d01138ab Update sixgill to ignore exception cleanup and properly qualify some type names https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9dedcf7163 Fix constructor recognition, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/19c13aa9b5ad Remove unused test files, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/788ac18818c9 Test for sixgill exception handling + analysis constructor/destructor matching, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/95107c3ad9cf Comments https://hg.mozilla.org/integration/mozilla-inbound/rev/a73f74f718e7 Annotate mFree function pointer as not GCing, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/c95bdd426ced Make ZoneCellIter variants to communicate nogc to the hazard analysis, r=jonco
Comment 43•8 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5cdcca45d9 warning fix on a CLOSED TREE
Various GC crashes started after this landed, all backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/69518db96a4d
Flags: needinfo?(sphink)
Comment 45•8 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae7a6263603 Switch to using a Map for visited points within a function, and heavily comment, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f523c94cf0 Rename function, add a few comments, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/a122f2f80ea9 The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/d91691bc2a5d Convert memoizer to Map, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7b3fc568de Refactoring: more Set/Map usage, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/46da36d5dd6b Start searching from the most specialized csu, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d28cf9c0cb Refactor findVirtualFunctions and improve comments, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/ab887355547d Hide command output behind --verbose flag, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8730c90dd1 Fix chunking implementation, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/4fac61d360ab In-source annotations for GC suppression, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/d4246cb8c2ba Various refactorings, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/c5817a915a31 Handle D4 destructors, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/8d85ed16c20e Provide GC stacks for synthesized functions, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/e51f08275a66 Rewrite the test suite, add several tests, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/810a121696ab Update sixgill to ignore exception cleanup and properly qualify some type names https://hg.mozilla.org/integration/mozilla-inbound/rev/4a346f93e840 Fix constructor recognition, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/7876b3fd15e6 Remove unused test files, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1752a35f57 Test for sixgill exception handling + analysis constructor/destructor matching, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/a682c8517601 Comments https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd10307c591 Annotate mFree function pointer as not GCing, r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/47dfe258f4b4 Make ZoneCellIter variants to communicate nogc to the hazard analysis, r=jonco
Assignee | ||
Comment 46•8 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #44) > Various GC crashes started after this landed, all backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/69518db96a4d Yes, my warning fix was invalid. I put #ifdef DEBUG around the code, but one part of it wasn't debug-only. :(
Flags: needinfo?(sphink)
Comment 47•8 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f17ed092065 trail of tears - more bogus reasons for calloc() to GC found. Suppress them.
Comment 48•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ae7a6263603 https://hg.mozilla.org/mozilla-central/rev/b9f523c94cf0 https://hg.mozilla.org/mozilla-central/rev/a122f2f80ea9 https://hg.mozilla.org/mozilla-central/rev/d91691bc2a5d https://hg.mozilla.org/mozilla-central/rev/4d7b3fc568de https://hg.mozilla.org/mozilla-central/rev/46da36d5dd6b https://hg.mozilla.org/mozilla-central/rev/d1d28cf9c0cb https://hg.mozilla.org/mozilla-central/rev/ab887355547d https://hg.mozilla.org/mozilla-central/rev/6b8730c90dd1 https://hg.mozilla.org/mozilla-central/rev/4fac61d360ab https://hg.mozilla.org/mozilla-central/rev/d4246cb8c2ba https://hg.mozilla.org/mozilla-central/rev/c5817a915a31 https://hg.mozilla.org/mozilla-central/rev/8d85ed16c20e https://hg.mozilla.org/mozilla-central/rev/e51f08275a66 https://hg.mozilla.org/mozilla-central/rev/810a121696ab https://hg.mozilla.org/mozilla-central/rev/4a346f93e840 https://hg.mozilla.org/mozilla-central/rev/7876b3fd15e6 https://hg.mozilla.org/mozilla-central/rev/dd1752a35f57 https://hg.mozilla.org/mozilla-central/rev/a682c8517601 https://hg.mozilla.org/mozilla-central/rev/2dd10307c591 https://hg.mozilla.org/mozilla-central/rev/47dfe258f4b4 https://hg.mozilla.org/mozilla-central/rev/6f17ed092065
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•