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

[CP] Handle Object members of dart:html types in dart2js runtime #48189

Closed
srujzs opened this issue Jan 21, 2022 · 5 comments
Closed

[CP] Handle Object members of dart:html types in dart2js runtime #48189

srujzs opened this issue Jan 21, 2022 · 5 comments
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve web-dart2js

Comments

@srujzs
Copy link
Contributor

srujzs commented Jan 21, 2022

commit(s) to merge: d172051

Please note that the only relevant non-test and non-status file changes are pkg/compiler/lib/src/js_emitter/native_emitter.dart and sdk/lib/_internal/js_runtime/lib/js_helper.dart, which amount to ~20 lines of code.

merge instructions: Cleanly cherry-picks to 455fe9d (the current beta version of 2.16.0-134.1.beta)

What is the issue: dart:html classes' type hierarchies were changed, but not fully accounted for in the dart2js runtime. This means we'd get incorrect values for some of the Object default methods e.g. hashCode.

What is the fix: The first change in the CL is to update the tags so dart2js can correctly register dart:html types. The second change is an additional if-clause to emit the right toString values.

Why cherrypick: The major component of this is hashCode. Maps that contain many instances of these types will hash them all in the same bucket, leading to performance regressions. Beyond this, and less importantly, toString and runtimeType result in significantly less useful values i.e. treating dart:html types as a generic JS object type.

Risk: Little.

The first change shouldn't make behavior worse than it already is, since it will only affect the dart:html types today. The second change only affects toString and error Strings when reporting a failed instance check.

Link to original issue(s): #48187

#47942 is where it first popped up.

/cc @mit-mit @whesse @athomas @vsmenon @devoncarew @sigmundch

@srujzs srujzs added web-dart2js area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-review Issue that need cherry pick triage to approve labels Jan 21, 2022
@srujzs srujzs changed the title [CP] Please add descriptive title for the pick here [CP] Handle Object members of dart:html types in dart2js runtime Jan 21, 2022
@sigmundch
Copy link
Member

I'm in favor of cherry-picking. This was an unintended regression and the cherry pick will bring 2.16 in line with the behavior and performance of 2.15.

Looking through the changes, the only actual code changes are down to 7 lines, the rest are comments and tests.

@devoncarew
Copy link
Member

Thanks for the cherry pick request! We're working through the approval now (I believe mostly related to potentially validating this change via a google3 roll).

@devoncarew devoncarew added the cherry-pick-approved Label for approved cherrypick request label Jan 21, 2022
@devoncarew
Copy link
Member

Updating this issue - this is now approved.

@srujzs
Copy link
Contributor Author

srujzs commented Jan 21, 2022

Thanks Devon!

@whesse
Copy link
Contributor

whesse commented Jan 24, 2022

Cherry-picked into 2.16.0-134.5.beta.

@whesse whesse closed this as completed Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve web-dart2js
Projects
None yet
Development

No branches or pull requests

4 participants