Closed Bug 817091 Opened 7 years ago Closed 7 years ago

GC: typedef Return<T> and T* to ReturnT

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

And remove the non-pointer bits from Return<T>.

Having Return<T> as an actual struct results in a reliable 3-5% performance hit on Kraken on all platforms in bug 811168 (and presumably in all the other places we've introduced Return<T>).  I verified this by manually removing all of the Return<T> from the patch in Bug 811168 and seeing the performance decrease disappear.
If we make our return types a struct in debug and and a pointer in opt, then NullPtr is not going to work as a return type.  What we really, really need here is nullptr_t.
Depends on: 806546
Attached patch v0Splinter Review
This is almost entirely mechanical after the gc/Root.h changes.
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #688040 - Flags: review?(wmccloskey)
Blocks: 816776
Comment on attachment 688040 [details] [diff] [review]
v0

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

This seems like a nice simplification without losing too much safety.

::: js/src/gc/Root.h
@@ +112,4 @@
>   *   scope. This can be used to force |v| out of scope before its C++ scope
>   *   would end naturally. The usage of braces C++ syntactical scopes |{...}|
>   *   is strongly perferred to this, but sometimes will not work because of
>   *   awkwardly overlapping lifetimes.

I think you need to update this comment to reflect the absence of Return<T>.

@@ +121,5 @@
> + * The following diagram explains the list of supported, implicit type
> + * conversions between classes of this family:
> + *
> + *  RawT <- - - - - - -+
> + *   v

Currently we can convert from Unrooted to Raw. That should be reflected in the diagram.

@@ +131,5 @@
> + *   +--------> Rooted<T> <----> Handle<T>
> + *                   ^              ^
> + *                   |              |
> + *                   |              |
> + *                   +---& MutableHandle<T>

Using the ampersand as the arrowhead is a little confusing. Maybe use a normal arrowhead but put the ampersand below the horizontal part of the arrow? Or, more explicitly, put (via &) below?

Also, is there any significance to the fact that some lines are dashed (between RawT and Rooted<T>) and the rest are not?

::: js/src/methodjit/PolyIC.cpp
@@ +1499,5 @@
>          }
>  
>          // Mark as not idempotent to avoid recompilation in Ion Monkey
>          // GetPropertyCache.
>          if (!obj->hasIdempotentProtoChain()) {

You can drop the braces.

@@ +1505,5 @@
>          }
>  
>          // The property is missing, Mark as not idempotent to avoid
>          // recompilation in Ion Monkey GetPropertyCache.
>          if (!getprop.holder) {

Same here.
Attachment #688040 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/5acd87d0cf33
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Duplicate of this bug: 810102
Depends on: 820215
You need to log in before you can comment on or make changes to this bug.