Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove array optimization path that prevents usage of setters on prototypes #3744

Closed
wants to merge 1 commit into from

Conversation

raskad
Copy link
Member

@raskad raskad commented Mar 17, 2024

This PR removes an array optimization path in the SetPropertyByValue opcode.

The removed code path would insert an indexed property into the dense indexed properties of an array, if the index is the next item in the dense indexed properties.
The problem is that if the prototype of the array contains a setter at the index, this setter is ignored.

For example:

Object.defineProperty(Array.prototype, '1', {
  set: function(param) {
    console.log("setter", param);
  }
});
let a = ["array"][1] = "value";

@raskad raskad added bug Something isn't working execution Issues or PRs related to code execution labels Mar 17, 2024
@raskad raskad added this to the v0.18.1 milestone Mar 17, 2024
@raskad
Copy link
Member Author

raskad commented Mar 17, 2024

@HalidOdat I would like to hear your opinion on the current solution of just removing the whole code path. The alternative would be to check the prototypes for an existing setter, but I think if we build that logic, we would just duplicate all of the code that the slow path does.
What do you think?

Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,268 50,268 0
Passed 42,773 42,774 +1
Ignored 1,391 1,391 0
Failed 6,104 6,103 -1
Panics 0 0 0
Conformance 85.09% 85.09% +0.00%
Fixed tests (1):
test/language/statements/for-in/head-lhs-let.js (previously Failed)

@HalidOdat
Copy link
Member

HalidOdat commented Mar 18, 2024

Hmmm... One thing we can do is to have a bit flag on the shape, whether the object has a setter (already have bitflags here for shared shapes), and just check the prototype's flag before preforming the optimization.

This may be useful for optimizing some of the object internal methods too, since some of them check for a setter/getter.

@HalidOdat
Copy link
Member

I'll try to work on the shape flag :)

@raskad
Copy link
Member Author

raskad commented Mar 24, 2024

Fixed in #3760

@raskad raskad closed this Mar 24, 2024
@raskad raskad deleted the fix-incorrect-array-optimization branch March 24, 2024 04:33
@raskad raskad removed this from the v0.18.1 milestone Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants