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

[JavaScript runtime] Implementation of ArrayPredictionContext.prototype.equals is incorrect #2902

Closed
ota-meshi opened this issue Sep 8, 2020 · 3 comments

Comments

@ota-meshi
Copy link

I checked that the JavaScript runtime is very slow compared to the Java runtime.
And I noticed that ArrayPredictionContext.prototype.equals rarely returns true.

Looking at the Java and JavaScript implementations, the Java implementation is comparing the array elements correctly, while the JavaScript implementation is comparing the array instances.

return this.returnStates === other.returnStates &&
this.parents === other.parents;

ArrayPredictionContext a = (ArrayPredictionContext)o;
return Arrays.equals(returnStates, a.returnStates) &&
Arrays.equals(parents, a.parents);

I think perhaps JS needs to be modified as follows:

    return equalArrays(this.returnStates, other.returnStates) &&
        equalArrays(this.parents, other.parents);

We can also use equalArrays in Utils.js, but we also need to change equalArrays in Utils.js because equalArrays in Utils.js doesn't allow arrays of primitives.

@ota-meshi
Copy link
Author

FYI, on my PC, the source code that took 10 seconds to parse was improved to less than 1 second after making this change.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Sep 8, 2020 via email

@ota-meshi
Copy link
Author

ota-meshi commented Sep 9, 2020

Thanks for your comment! I'm looking forward to the results you look into this.

As additional information, I think that the cache existence judgment failed, the cache was not used, and it was always recalculated, but this change made it use the cache, so it was faster.

@parrt parrt closed this as completed in ef029ce Sep 15, 2020
ericvergnaud added a commit that referenced this issue Dec 6, 2020
…-incorrect-fix

rollback partially incorrect fix for #2902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants