Skip to content

Comments

perf(syntax): reorder operator enum variants#7351

Merged
graphite-app[bot] merged 1 commit intomainfrom
11-18-perf_syntax_reorder_operator_enum_variants
Nov 19, 2024
Merged

perf(syntax): reorder operator enum variants#7351
graphite-app[bot] merged 1 commit intomainfrom
11-18-perf_syntax_reorder_operator_enum_variants

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Nov 18, 2024

Re-order enum variants of AssignmentOperator, BinaryOperator and UnaryOperator.

  • Exponential moved to after Remainder (so with the rest of the arithmetic operators).
  • Shift* operators follow arithmetic operators.
  • AssignmentOperator::Bitwise* ops moved to before Logical* ops (so all ops which correspond to BinaryOperators are together).
  • *Or always before *And.
  • Plus/Addition always before Minus/Subtraction.

The purpose is to make the various methods on these types maximally efficient:

  1. Group together variants so that AssignmentOperator::is_* methods can be executed with the minimum number of operations (essentially variant - min <= max).
  2. Align the variants of AssignmentOperator and BinaryOperator so that conversion methods added in feat(syntax): add AssignmentOperator::to_logical_operator and to_binary_operator methods #7350 become very cheap too (essentially if variant - min <= max { Some(variant + offset) } else { None }).

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 18, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the C-performance Category - Solution not expected to change functional behavior, only performance label Nov 18, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 18, 2024

CodSpeed Performance Report

Merging #7351 will not alter performance

Comparing 11-18-perf_syntax_reorder_operator_enum_variants (c335f92) with main (bc0e72c)

Summary

✅ 30 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 11-18-feat_syntax_add_assignmentoperator_to_logical_operator_and_to_binary_operator_methods branch 2 times, most recently from 890bdfa to 53997c2 Compare November 18, 2024 22:32
@overlookmotel overlookmotel force-pushed the 11-18-perf_syntax_reorder_operator_enum_variants branch from 03d2dad to 2ce3a79 Compare November 18, 2024 22:32
@overlookmotel overlookmotel marked this pull request as ready for review November 18, 2024 22:33
@overlookmotel overlookmotel force-pushed the 11-18-perf_syntax_reorder_operator_enum_variants branch from 2ce3a79 to 82b8c74 Compare November 18, 2024 22:37
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 19, 2024
Copy link
Member

Boshen commented Nov 19, 2024

Merge activity

  • Nov 18, 8:08 PM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 18, 8:08 PM EST: A user added this pull request to the Graphite merge queue.
  • Nov 18, 8:34 PM EST: A user merged this pull request with the Graphite merge queue.

@Boshen Boshen force-pushed the 11-18-feat_syntax_add_assignmentoperator_to_logical_operator_and_to_binary_operator_methods branch from 53997c2 to 2534cde Compare November 19, 2024 01:12
Boshen pushed a commit that referenced this pull request Nov 19, 2024
Re-order enum variants of `AssignmentOperator`, `BinaryOperator` and `UnaryOperator`.

* `Exponential` moved to after `Remainder` (so with the rest of the arithmetic operators).
* `Shift*` operators follow arithmetic operators.
* `AssignmentOperator::Bitwise*` ops moved to before `Logical*` ops (so all ops which correspond to `BinaryOperator`s are together).
* `*Or` always before `*And`.
* Plus/Addition always before Minus/Subtraction.

The purpose is to make the various methods on these types maximally efficient:

1. Group together variants so that `AssignmentOperator::is_*` methods can be executed with the minimum number of operations (essentially `variant - min <= max`).
2. Align the variants of `AssignmentOperator` and `BinaryOperator` so that conversion methods added in #7350 become very cheap too (essentially `if variant - min <= max { Some(variant + offset) } else { None }`).
@Boshen Boshen force-pushed the 11-18-perf_syntax_reorder_operator_enum_variants branch from 82b8c74 to 8e3adab Compare November 19, 2024 01:12
Base automatically changed from 11-18-feat_syntax_add_assignmentoperator_to_logical_operator_and_to_binary_operator_methods to main November 19, 2024 01:17
Re-order enum variants of `AssignmentOperator`, `BinaryOperator` and `UnaryOperator`.

* `Exponential` moved to after `Remainder` (so with the rest of the arithmetic operators).
* `Shift*` operators follow arithmetic operators.
* `AssignmentOperator::Bitwise*` ops moved to before `Logical*` ops (so all ops which correspond to `BinaryOperator`s are together).
* `*Or` always before `*And`.
* Plus/Addition always before Minus/Subtraction.

The purpose is to make the various methods on these types maximally efficient:

1. Group together variants so that `AssignmentOperator::is_*` methods can be executed with the minimum number of operations (essentially `variant - min <= max`).
2. Align the variants of `AssignmentOperator` and `BinaryOperator` so that conversion methods added in #7350 become very cheap too (essentially `if variant - min <= max { Some(variant + offset) } else { None }`).
@Boshen Boshen force-pushed the 11-18-perf_syntax_reorder_operator_enum_variants branch from 8e3adab to c335f92 Compare November 19, 2024 01:23
@graphite-app graphite-app bot merged commit c335f92 into main Nov 19, 2024
@graphite-app graphite-app bot deleted the 11-18-perf_syntax_reorder_operator_enum_variants branch November 19, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants