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

Speed up record inclusion check. #6289

Merged
merged 4 commits into from
Jun 7, 2023
Merged

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Jun 6, 2023

Speed up record inclusion check.
Fixes #6284

Record inclusion check (between implementation and interface) is quadratic.

Example:

module M : {
  type t<'a, 'b, 'c> = {x:int, y:list<('a, 'b)>, z:int}
} = {
  type t<'a, 'b, 'c> = {x:int, y:list<('a, 'c)>, z:int}
}

The existing algorithm tries to instantiate type parameters. It only reports an error if there is an inconsistency. This requires solving type equations involving many types at once.

To give an accurate error message, the first problematic field is reported (in this case y). So the type equations are checked again and again with size 1, 2, ...n where n is the number of fields. (Plus the type parameters).

This is quadratic and is problematic for records of ~1K fields.

This PR provides a fast path which just checks if there is an error, without blaming a specific field. The fast path is linear.
Only if an error is detected, the quadratic path is take to blame precisely which field is involved.

Copy link
Member

@mununki mununki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clever!
This PR actually makes me wonder why upstream does Ctype.equal nested. Obviously, for a record with many fields, it would be very slow.

@cknitt
Copy link
Member

cknitt commented Jun 6, 2023

Great work! 🎉

There is another place where compare_records is invoked (in compare_constructor_arguments). Should that be adapted, too?

@zth
Copy link
Collaborator

zth commented Jun 6, 2023

@mununki could you check what https://github.com/mununki/benchmark-rescript-records gives when running this PR?

@mununki
Copy link
Member

mununki commented Jun 7, 2023

@mununki could you check what mununki/benchmark-rescript-records gives when running this PR?

It's clear that performance has improved under the same conditions.

# before
Normal record x 7.29 ops/sec ±0.96% (23 runs sampled)
Record with spread x 5.30 ops/sec ±1.20% (18 runs sampled)
Record with optional x 5.08 ops/sec ±3.61% (17 runs sampled)
Record with spread and optional x 3.37 ops/sec ±0.96% (13 runs sampled)

# after
Normal record x 8.55 ops/sec ±0.83% (26 runs sampled)
Record with spread x 6.57 ops/sec ±2.79% (21 runs sampled)
Record with optional x 7.97 ops/sec ±1.18% (25 runs sampled)
Record with spread and optional x 6.40 ops/sec ±0.46% (20 runs sampled)

Speed up record inclusion check.
Fixes #6284

Record inclusion check (between implementation and interface) is quadratic.

Example:
```res
module M : {
  type t<'a, 'b, 'c> = {x:list<('a, 'b)>, y:int, z:int}
} = {
  type t<'a, 'b, 'c> = {x:list<('a, 'c)>, y:int, z:int}
}
```

The algorithm tries to instantiate type parameters. It only reports an error if there is an inconsistency. This requires solving type equations involving many types at once.

To improve error message, the first problematic field is reported. So the type equations are checked again and again with size 1, 2, ...n where n is the number of fields. (Plus the type parameters).

This is quadratic and is problematic for types of ~1K elements.

This PR provides a fast path which just checks if there is an error, without blaming a specific field.
The fast path is linear.
Only if an error is detected, the quadratic path is take to blame precisely which field is involved.
@cristianoc cristianoc force-pushed the speed_up_record_inclusion_check branch from b335eef to cda4816 Compare June 7, 2023 07:24
So that there's some minimal sanity check that record type inclusion works as expected on a nontrivial case.
@cristianoc
Copy link
Collaborator Author

Great work! 🎉

There is another place where compare_records is invoked (in compare_constructor_arguments). Should that be adapted, too?

Refactored so there's a single function now.

@cristianoc
Copy link
Collaborator Author

One alternative would be to make this internal function incremental:

let eqtype_list rename type_pairs subst env tl1 tl2 =
  univar_pairs := [];
  let snap = Btype.snapshot () in
  try eqtype_list rename type_pairs subst env tl1 tl2; backtrack snap
  with exn -> backtrack snap; raise exn

so it would not slow down even for the error case. However it would be more complicated.

@cristianoc cristianoc merged commit a053b62 into master Jun 7, 2023
7 checks passed
@cristianoc cristianoc deleted the speed_up_record_inclusion_check branch June 7, 2023 07:51
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

Successfully merging this pull request may close these issues.

Using JsxDOM.domProps in a type spread seems to be slow (discovered in playground)
4 participants