GC: remove skip-roots from NewString function family

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
The NewString functions need to allocate (and therefore GC) before using their jschar* argument.  If we make the NewString functions take StableCharPtr, we will basically kill off our ability to ever use inline strings.  Since these functions just need to be able to find the base of the character buffer after a GC, the right solution is some form of RelocatableCharPtr.
(Assignee)

Comment 1

6 years ago
Waldo points out that NewString should just take a JSString/offset/length in any of the places where we do not already have a StableCharPtr/StableString.  I concur that this is a better solution.
(Assignee)

Comment 2

6 years ago
Created attachment 672040 [details] [diff] [review]
v0

This patch contains a couple of simple, but very helpful cleanups.

1) Create a new StableCharPtr for each of the jschar* JS-API inputs, similar to how we root everything internally.  We were already doing this in an ad-hoc manner for the interfaces that require StableCharPtr, this just makes everything consistent.

2) Convert all of the optionally NULL |cx| parameters in the Inflate/Deflate function family to be named |maybecx|.  This also moves the local variable declarations down to where they should be in C++.

3) PodCopy/pod_malloc in js_NewStringCopyZ.

Sorry to hand you such a boring, mechanical review, but I wanted to keep these changes from cluttering up the real patch, which I will attach shortly.
Attachment #672040 - Flags: review?(luke)
(Assignee)

Comment 3

6 years ago
Created attachment 672042 [details] [diff] [review]
v0: The fixes.

This is the first half of making our JSString creation functions take StableCharPtrs.  The second half is going to require significant and orthogonal changes to Atomize, so I've split it off into bug 802318.
Attachment #672042 - Flags: review?(luke)
(Assignee)

Comment 4

6 years ago
Comment on attachment 672042 [details] [diff] [review]
v0: The fixes.

Hurm, try run is busted, I'd better investigate first.
Attachment #672042 - Flags: review?(luke)
(Assignee)

Comment 5

6 years ago
Created attachment 672620 [details] [diff] [review]
v1

Looks green now: https://tbpl.mozilla.org/?tree=Try&rev=dc3604cbc8fe

The failure was that I was not updating the start offset when climbing to the most basal dependent string in js_NewDependentString.  This version also adds StableCharPtr for JSExtendedString, which I missed before, and pushes StableCharPtr into the various init methods.

The last step now is the make pod_malloc<T>(size_t) return a RangedPtr<T>.  I still need to figure out how to make RangedPtr handle the |const jschar| vs |jschar| distinction elegantly.  Other than that, it looks fairly straightforward to do this now, given that most of the users flow directly into a StableCharPtr.
Attachment #672042 - Attachment is obsolete: true
Attachment #672620 - Flags: review?(luke)
This has been bugging me for a while and Waldo was thinking the same thing:

When an API logically wants to take (pointer, length), and then we go off and make it safe, so we have to write (RangedPtr(pointer, length), length).  The second patch here does this all over the place.

So, my suggesting: wouldn't it be nice if we could have a new Range<T> type that represented a (pointer, length).  Then, it could have a .ptr() method that returned a RangedPtr<T> and a .length().  Since the above patches really wants Range<T> to not look dorky, could we do that as a prereq for this bug?
(Assignee)

Comment 7

6 years ago
I've been thinking about doing this myself.  I held off because I was going to make pod_malloc(size_t) return RangedPtr next.  This would kill off almost all of these places, leaving us with a rather minimal interface diff from |const jschar *| to |StableCharPtr|.  

If you're asking for this change now, however....  I'll just go polish my code-carving tools for a bit.

One question I have is: Is Range<T> the best name for this tuple?  IMHO, I'd rather put the length directly into the CharPtr heirarchy.  It's less general, but I think CharPtr is a broadly applicable enough class that it warrants this treatment.
I'd expect:

  RangedPtr <-- CharPtr <-- StableCharPtr
  Range <-- CharRange <-- StableCharRange
(Assignee)

Updated

6 years ago
Attachment #672620 - Flags: review?(luke)
(Assignee)

Comment 9

6 years ago
Comment on attachment 672040 [details] [diff] [review]
v0

Whoops, sorry I left this flagged so long, Luke.
Attachment #672040 - Attachment is obsolete: true
Attachment #672040 - Flags: review?(luke)
(Assignee)

Updated

6 years ago
Depends on: 811060
(Assignee)

Updated

6 years ago
Blocks: 813244
No longer blocks: 773686
Whiteboard: [js:t]
(Assignee)

Comment 10

6 years ago
Created attachment 702587 [details] [diff] [review]
v2

Now that TwoByteChars has landed, we can do this much more simply. This extends the existing infrastructure just enough to allow us to safely create short strings from Unstable chars.
Attachment #672620 - Attachment is obsolete: true
Attachment #702587 - Flags: review?(wmccloskey)
Comment on attachment 702587 [details] [diff] [review]
v2

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

Looks nice, thanks.

::: js/public/CharacterEncoding.h
@@ +94,5 @@
>      TwoByteChars(const jschar *chars, size_t length) : Base(const_cast<jschar *>(chars), length) {}
>  };
>  
>  /*
> + * A non-convertible varient of TwoByteChars that does not refer to characters

variant

::: js/src/vm/String-inl.h
@@ +21,5 @@
>  
>  namespace js {
>  
>  static JS_ALWAYS_INLINE JSInlineString *
> +NewShortString(JSContext *cx, Latin1Chars chars)

Why does this need to be inline when it wasn't before?

@@ +76,5 @@
> +    JSInlineString *str = JSInlineString::lengthFits(len)
> +                          ? JSInlineString::tryNew_(cx)
> +                          : JSShortString::tryNew_(cx);
> +    if (!str) {
> +        ScopedFreePtr<jschar> tmp(cx->pod_malloc<jschar>(len));

Why malloc? Can't you stack allocate it?
Attachment #702587 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 12

6 years ago
(In reply to Bill McCloskey (:billm) from comment #11)
> Comment on attachment 702587 [details] [diff] [review]
> ::: js/src/vm/String-inl.h
> @@ +21,5 @@
> >  
> >  namespace js {
> >  
> >  static JS_ALWAYS_INLINE JSInlineString *
> > +NewShortString(JSContext *cx, Latin1Chars chars)
> 
> Why does this need to be inline when it wasn't before?

Yeah, this is pretty annoying. For reasons I don't entirely understand, without the inline clang gives an unused function warning on every translation unit vm/String-inl.h gets included into except for jsstr.cpp, where it is used.

Our options are:
 - Move JSDependentString::new_ out of the header so that we can move NewShortString out of headers as well.
 - Move NewShortString(Latin1) back to where it was, not alongside its namesake.
 - Make it inline. This doesn't hurt us since it only has one caller anyway.

I think that inlining is probably the best tradeoff here.
https://hg.mozilla.org/mozilla-central/rev/27f62505f15c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.