Skip to content

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Oct 2, 2024

Experiment. Not for merging.

As an alternative to #11742, we could wrap state in common transforms in UnsafeCell instead of RefCell. We'd need a nice way to do that (probably would have to be a proc macro, unfortunately). This PR just does it manually with an ugly method, to see what the perf gain is, and so how worthwhile it would be to do it properly.

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 2, 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.

Copy link
Member Author

overlookmotel commented Oct 2, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Oct 2, 2024
@overlookmotel overlookmotel force-pushed the 10-02-refactor_transformer_move_functionalty_of_common_transforms_into_stores branch from 6a632df to a0a3a8b Compare October 2, 2024 11:43
@overlookmotel overlookmotel force-pushed the 10-02-perf_transformer_common_transforms_store_state_in_unsafecell_ branch from 3c7e1aa to e2f2ec8 Compare October 2, 2024 11:43
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 2, 2024

CodSpeed Performance Report

Merging #6246 will not alter performance

Comparing 10-02-perf_transformer_common_transforms_store_state_in_unsafecell_ (810cfb5) with 10-02-refactor_transformer_move_functionalty_of_common_transforms_into_stores (a0a3a8b)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 10-02-perf_transformer_common_transforms_store_state_in_unsafecell_ branch from e2f2ec8 to 810cfb5 Compare October 2, 2024 12:05
@overlookmotel
Copy link
Member Author

Only +0.2% perf boost at best. I'm really surprised.

Perhaps I've done it wrong, or perhaps compiler was able to deduce that RefCell::borrow_mut could never panic, and optimize out the checks, so using UnsafeCell offers little advantage.

@Boshen Boshen deleted the 10-02-perf_transformer_common_transforms_store_state_in_unsafecell_ branch February 17, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-transformer Area - Transformer / Transpiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants