Last Comment Bug 723346 - GC: make sharpObjectMap a modern HashTable
: GC: make sharpObjectMap a modern HashTable
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Terrence Cole [:terrence]
: Jason Orendorff [:jorendorff]
Depends on: 724586
  Show dependency treegraph
Reported: 2012-02-01 16:30 PST by Terrence Cole [:terrence]
Modified: 2012-02-17 05:12 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

V1: minimal (14.53 KB, patch)
2012-02-08 18:37 PST, Terrence Cole [:terrence]
jwalden+bmo: review+
Details | Diff | Splinter Review
v2: With review feedback. (15.96 KB, patch)
2012-02-16 11:27 PST, Terrence Cole [:terrence]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-02-01 16:30:13 PST
This has the same problem as other places where we have markable, nursery allocable objects as hash table keys: we may need to move them about during GC.  We should, at the very least, replace this old table with a new one so that we only have to solve the problems in one place.

Another option here is to rip out the recursive toSource.  It's not part of any spec and if it were, it would most likely want to throw on recursion.
Comment 1 Nicholas Nethercote [:njn] 2012-02-02 01:21:41 PST
Does sharpObjectMap have anything to do with sharps, which Waldo just removed support for?  I looked at the code but am none the wiser.
Comment 2 Jason Orendorff [:jorendorff] 2012-02-02 04:50:35 PST
Yes, but now it's only used for toSource. (It might as well be renamed at the same time.)
Comment 3 Terrence Cole [:terrence] 2012-02-08 18:37:11 PST
Created attachment 595607 [details] [diff] [review]
V1: minimal

I ended up doing this in as minimal a fashion as possible.  It turns out that this code path relies on at least 4 different bools, an int, and a pointer treated as a bool.  I printed these out for examples in the test suite and it seems that they all do have different, specific purposes.

AFAICT the relationship looks roughly like:
  hasGen = !hasGen && !isSharp
  alreadySeen = hasGen || isSharp
  ida = map->depth == 0 && !isSharp
  isSharp = alreadySeen && ida
  ++map->depth when !isSharp
  --map->depth unconditionally (not sure how this works, but I added asserts)

I'm fairly certain this could be untwisted further, but it's beyond the scope of this bug.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-15 14:52:44 PST
Comment on attachment 595607 [details] [diff] [review]
V1: minimal

Review of attachment 595607 [details] [diff] [review]:

I don't have much confidence in my review of this, but it looks like it's the right translation.  I think.  Keep an eye out for regressions, and sic the fuzzers on this if you can.

::: js/src/jsobj.cpp
@@ +358,5 @@
>      if (idap)
>          *idap = ida;
> +    if (isSharp)
> +        *isSharp = sharpid.isSharp;

I don't believe it's the case that |isSharp| can ever be NULL -- just do this assignment unconditionally.

@@ +416,5 @@
>       * to justify spending efforts.
>       */
> +    for (JSSharpTable::Range r = map->table.all(); !r.empty(); r.popFront()) {
> +        MarkRoot(trc, r.front().key, "sharp table entry");
> +    }

Don't brace this single-line body.

@@ +446,5 @@
>      JSIdArray *ida;
>      bool alreadySeen = false;
> +    bool isSharp = false;
> +    bool success = js_EnterSharpObject(cx, obj, &ida, &alreadySeen, &isSharp);
> +    if (!success)

Eliminate the unnecessary |bool success| temporary here.

@@ +466,5 @@
> +    JS_ASSERT(!isSharp);
> +    if (alreadySeen) {
> +        JSSharpTable::Ptr p = cx->sharpObjectMap.table.lookup(obj);
> +        JS_ASSERT(p);
> +        JS_ASSERT(p->value.isSharp == isSharp);

Make that |!p->value.isSharp|.
Comment 5 Terrence Cole [:terrence] 2012-02-16 11:27:48 PST
Created attachment 597916 [details] [diff] [review]
v2: With review feedback.
Comment 6 Terrence Cole [:terrence] 2012-02-16 11:29:56 PST
Gary, would it be possible to get some fuzzing on Object.toSource and Array.toSource with this patch applied?
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2012-02-16 11:39:29 PST
> Gary, would it be possible to get some fuzzing on Object.toSource and
> Array.toSource with this patch applied?

Sure - Christian might be interested too. :)
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2012-02-16 14:34:33 PST
> Gary, would it be possible to get some fuzzing on Object.toSource and
> Array.toSource with this patch applied?

This seems to hold up well after an hour or two's worth of fuzzing, in other words the patch didn't blow up the fuzzer.
Comment 9 Terrence Cole [:terrence] 2012-02-16 14:45:58 PST

Thanks for the fuzzing!
Comment 10 Ed Morley [:emorley] 2012-02-17 05:12:38 PST

Note You need to log in before you can comment on or make changes to this bug.