Closed Bug 1179315 Opened 5 years ago Closed 1 year ago

eliminate static constructors related to TypedArray bits

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(3 files)

No description provided.
The definitions of {Shared,}TypedArrayObject::layout_ require static
constructors on some compilers because they can't see the full
definition of TypedArrayLayout's constructor.  We can address this by
moving the constructor to a point where it can be easily inlined, and
marking it as MOZ_CONSTEXPR.

Lars, throwing this and following at you, since I know you've done work in this
area recently, but please redirect to compensate for my ignorance if necessary.
Attachment #8628331 - Flags: review?(lhansen)
We need MOZ_CONSTEXPR on {Shared,}TypedArrayObject::ArrayTypeID for some
compilers to be able to constant-fold that function.  But said compilers
didn't seem to understand MOZ_CONSTEXPR annotations on TypeIDOfType,
either on the template declaration or the individual specializations.
Instead, we convert TypeIDOfType into a traits template, so ArrayTypeID
can return a logical constant instead.

It's unfortunate that MOZ_CONSTEXPR annotations on the template and
specializations don't fix this; it seems that specialized functions instead of
traits structures are the JS Way, but I don't see how to do the function
approach.
TypedArrayObject::protoClasses uses this function to initialize bits of
itself from TypedArrayObject::classes.  On some compilers, this function
call requires a static constructor.  For the benefit of those compilers,
use MOZ_CONSTEXPR so the compiler can constant-fold the call and avoid
the static constructor.
Attachment #8628333 - Flags: review?(lhansen)
Comment on attachment 8628332 [details] [diff] [review]
part 2 - make it more obvious that typeIDs of typed arrays are constants

Argh, forgot to set the r? flag on this one.  Please see comment 2 for additional context.
Attachment #8628332 - Flags: review?(lhansen)
Comment on attachment 8628331 [details] [diff] [review]
part 1 - make TypedArrayLayout's constructor inline and MOZ_CONSTEXPR

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

::: js/src/vm/TypedArrayObject.h
@@ +45,5 @@
>      const Class* maxClass_;
>  
>    public:
> +    MOZ_CONSTEXPR TypedArrayLayout(bool isShared, bool isNeuterable, const Class* firstClass, const Class* maxClass)
> +        : isShared_(isShared), isNeuterable_(isNeuterable), firstClass_(firstClass), maxClass_(maxClass)

Those lines are too long (100 chars max) and should be split, cf the code that was removed from TypedArrayObject.cpp.
Attachment #8628331 - Flags: review?(lhansen) → review+
Attachment #8628332 - Flags: review?(lhansen) → review+
Attachment #8628333 - Flags: review?(lhansen) → review+
Part 3 is a no-go with our current compilers, because they don't understand that reinterpret_cast + static_cast is actually constexpr. :(
Keywords: leave-open
The leave-open keyword is there and there is no activity for 6 months.
:froydnj, maybe it's time to close this bug?
Flags: needinfo?(nfroyd)
(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #9)
> The leave-open keyword is there and there is no activity for 6 months.
> :froydnj, maybe it's time to close this bug?

This was left open to fix when our compilers caught up to C++, but I see the code has been changed in the meantime.  Let's call this fixed, and we can open a different bug if need be.
Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(nfroyd)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.