723346 - GC: make sharpObjectMap a modern HashTable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

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 [inactive]]

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]]

Yes, but now it's only used for toSource. (It might as well be renamed at the same time.)

Comment 3

[Terrence Cole [:terrence]]

Attachment: 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]]

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]]

Attachment: v2: With review feedback.

Comment 6

[Terrence Cole [:terrence]]

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] (NOT official MoCo now)]

> 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] (NOT official MoCo now)]

> 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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae2b959b51d

Thanks for the fuzzing!

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ae2b959b51d