BaselineCompiler: Minor cleanup

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
6 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Posted patch PatchSplinter Review
I've had this patch in my queue for a while. Removes some TODO comments (looks like we don't need megamorphic stubs), and lowers the max number of stubs we attach for some IC's.
Attachment #732975 - Flags: review?(kvijayan)
Comment on attachment 732975 [details] [diff] [review]
Patch

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

::: js/src/ion/BaselineIC.cpp
@@ -1012,5 @@
>      JS_ASSERT_IF(wasDetachedMonitorChain, numOptimizedMonitorStubs_ == 0);
>  
> -    if (numOptimizedMonitorStubs_ >= MAX_OPTIMIZED_STUBS) {
> -        // TODO: if the TypeSet becomes unknown or has the AnyObject type,
> -        // replace stubs with a single stub to handle these.

I would like to leave this TODO here.  But first we want to reduce the maximum type object count held by a TypeSet down to something reasonable (like, 32 instead of the current 256).

@@ -1244,5 @@
>                                       RawId id, HandleValue val)
>  {
> -    if (numOptimizedStubs_ >= MAX_OPTIMIZED_STUBS) {
> -        // TODO: if the TypeSet becomes unknown or has the AnyObject type,
> -        // replace stubs with a single stub to handle these.

Same as above.

::: js/src/ion/BaselineIC.h
@@ +4479,2 @@
>      static const uint32_t MAX_SCRIPTED_STUBS = 7;
>      static const uint32_t MAX_NATIVE_STUBS = 7;

I think a better change for this would be to do:

MAX_SCRIPTED_STUBS=5
MAX_NATIVE_STUBS=5
MAX_OPTIMIZED_STUBS = MAX_SCRIPTED_STUBS + MAX_NATIVE_STUBS + 1;

This will always leave opportunity for native and scripted call stubs to collapse into a generic.
Attachment #732975 - Flags: review?(kvijayan) → review+
Any plan on landing this patch?  Is this bug still valid?
Flags: needinfo?(kvijayan)
(Assignee)

Comment 3

6 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Any plan on landing this patch?  Is this bug still valid?

Yup I will try to land it this week.
Flags: needinfo?(kvijayan)
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.