Clean up string marking

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 8597464 [details] [diff] [review]
10.7_clean_up_string_marking-v0.diff

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+
(Assignee)

Comment 2

4 years ago
(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.

Updated

4 years ago
Depends on: 1160149
https://hg.mozilla.org/mozilla-central/rev/75d6145145ec
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
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.