GC: make sharpObjectMap a modern HashTable

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Does sharpObjectMap have anything to do with sharps, which Waldo just removed support for?  I looked at the code but am none the wiser.
Yes, but now it's only used for toSource. (It might as well be renamed at the same time.)
(Assignee)

Updated

6 years ago
Depends on: 724586
(Assignee)

Comment 3

6 years ago
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.
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #595607 - Flags: review?(jwalden+bmo)
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|.
Attachment #595607 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 597916 [details] [diff] [review]
v2: With review feedback.
Attachment #595607 - Attachment is obsolete: true
Attachment #597916 - Flags: review+
(Assignee)

Comment 6

5 years ago
Gary, would it be possible to get some fuzzing on Object.toSource and Array.toSource with this patch applied?
> 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. :)
> 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.
(Assignee)

Comment 9

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

Thanks for the fuzzing!
https://hg.mozilla.org/mozilla-central/rev/8ae2b959b51d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.