Skip to content

Conversation

@MichaReiser
Copy link
Contributor

Similar to other events, use DatabaseKey for interned events.
This has the advantage that logging the event reveals the id and the struct name

Test plan

See updated snapshot tests

@netlify
Copy link

netlify bot commented Apr 24, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 10f5832
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/680a0ef7e02d240008ad87b3

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 24, 2025

CodSpeed Performance Report

Merging #813 will improve performances by 6.79%

Comparing MichaReiser:interned-events-key (10f5832) with master (201a8dd)

Summary

⚡ 2 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[Input] 3.4 µs 3.2 µs +4.11%
amortized[SupertypeInput] 4.1 µs 3.8 µs +6.79%

@MichaReiser
Copy link
Contributor Author

I very much question the performance improvements 😆

@MichaReiser MichaReiser force-pushed the interned-events-key branch from f6bff96 to 6aafb95 Compare April 24, 2025 10:06
@MichaReiser MichaReiser force-pushed the interned-events-key branch from 6aafb95 to cae027a Compare April 24, 2025 10:10
@Veykril
Copy link
Member

Veykril commented Apr 24, 2025

The dynamically dispatched event call is likely affecting inlining in a variety of ways (sometimes good sometimes bad), certainly contributes to the noise at times

@MichaReiser MichaReiser force-pushed the interned-events-key branch from cae027a to 10f5832 Compare April 24, 2025 10:14
@MichaReiser MichaReiser added this pull request to the merge queue Apr 24, 2025
Merged via the queue into salsa-rs:master with commit f7eea9d Apr 24, 2025
11 checks passed
@MichaReiser MichaReiser deleted the interned-events-key branch April 24, 2025 10:49
@github-actions github-actions bot mentioned this pull request May 2, 2025
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.

2 participants