Closed Bug 1158353 Opened 9 years ago Closed 9 years ago

Clean up string marking

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Using the same pattern we used for LazyScript and Shape gives us this patch.
Attachment #8597464 - Flags: review?(jcoppeard)
Comment on attachment 8597464 [details] [diff] [review]
10.7_clean_up_string_marking-v0.diff

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

I found the the paired functions in this patch confusing to start with.  Can we have a comment explaining that they are the generic trace and eager marking paths and that they (mainly) correspond to each other?

::: js/src/gc/Marking.cpp
@@ +788,5 @@
> +{
> +    if (str->isLinear())
> +        eagerlyMarkChildren(&str->asLinear());
> +    else
> +        eagerlyMarkChildren(&str->asRope());

Pre-exisiting, but what happens about other kinds of string?  I guess we don't trace those on our eager path.  Can we insert a comment and assertion here to that effect?

This also makes the two trace paths not equivalent here.
Attachment #8597464 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8597464 [details] [diff] [review]
> 10.7_clean_up_string_marking-v0.diff
> 
> Review of attachment 8597464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I found the the paired functions in this patch confusing to start with.  Can
> we have a comment explaining that they are the generic trace and eager
> marking paths and that they (mainly) correspond to each other?

Yes, that should be better explained. I also should have moved the rest of the marking related stuff up sooner. I'll try to do that today.

> ::: js/src/gc/Marking.cpp
> @@ +788,5 @@
> > +{
> > +    if (str->isLinear())
> > +        eagerlyMarkChildren(&str->asLinear());
> > +    else
> > +        eagerlyMarkChildren(&str->asRope());
> 
> Pre-exisiting, but what happens about other kinds of string?  I guess we
> don't trace those on our eager path.  Can we insert a comment and assertion
> here to that effect?

Actually, no, there's no smoke and mirrors here: none of the other string kinds contain children, so there are none to mark here.
 
> This also makes the two trace paths not equivalent here.

I don't think there are any other string trace functions.
Depends on: 1160149
https://hg.mozilla.org/mozilla-central/rev/75d6145145ec
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to Terrence Cole [:terrence] from comment #2) 
> Actually, no, there's no smoke and mirrors here: none of the other string
> kinds contain children, so there are none to mark here.

I was worried about marking the base pointer in JSUndependedString but didn't spot that it ultimately derives from JSLinearString, so this is all good.
You need to log in before you can comment on or make changes to this bug.