1370831 - Investigate removing Add/RemoveRawValueRoot API

Status: NEW

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

This is only used in a couple of places (ErrorResult and in XPConnect).  It would be great if we could remove this API and use something else for these instead.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Sorry if I ask stupied question, so in this bug, should we try to replace ErrorResult.mJSException with PersistentRooted<Value>?
And for PersistentRooted, we don't have to unroot it, so we simply remove call to RemoveRawValueRoot?

Thanks

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Yoshi Huang [:allstars.chh] from comment #1)
> Sorry if I ask stupied question, so in this bug, should we try to replace
> ErrorResult.mJSException with PersistentRooted<Value>?
> And for PersistentRooted, we don't have to unroot it, so we simply remove
> call to RemoveRawValueRoot?

It isn't a stupid question. I'm not an expert on this code, but I suspect the reason it isn't just a PersistentRooted<> is to avoid the overhead of rooting in the (common?) case when mJSException is not set. I think this class is very performance sensitive. Maybe something like Maybe<PersistentRooted<Value>> would avoid the overhead. If that is allowed.

Comment 3

[Andrew McCreight [:mccr8]]

This API is also used in two places in the Servo code we have in the tree, but it looks like it is for DOM bindings for callbacks and promises, so I think it would only affect Servo, not Firefox.

The XPConnect code that uses this API doesn't look super performance sensitive. Somebody would have to figure out what this XPTCVariant thing is doing and why.

Comment 4

[Boris Zbarsky [:bzbarsky]]

One of the design goals for ErrorResult was to be zero-cost, or as close as possible, when no exception is thrown (which is by far the common case).  The context was bindings code, where we were literally counting every CPU cycle.

That means a no-op (in opt builds) inlined destructor and a constructor that does the smallest amount of work possible (just inits mResult to NS_OK).

PersistentRooted fails the zero-cost design criterion badly, obviously.  Maybe<PersistentRooted> fails it too, by introducing extra branches and extra writes.

Now maybe we can live with those, especially on x86 where branch predictors work pretty well (unlike ARM).  We'd need to measure carefully.

Of course the tradeoff is that you _can_ leak via an ErrorResult if you're not careful and we have all sorts of assertions to catch code that might do that...  More RAII would let us have a bit more safety here.