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.

Sign up for GitHub

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

Jump to bottom

perf: use content-visibility for custom emoji #445

Merged
merged 6 commits into from
Sep 7, 2024
Merged

perf: use content-visibility for custom emoji #445

merged 6 commits into from
Sep 7, 2024

Conversation

nolanlawson
Copy link
Owner

@nolanlawson nolanlawson commented Sep 7, 2024 •
edited
Loading

Rather than using list virtualization (which has accessibility and complexity issues), we can use content-visibility to hide offscreen categories of custom emoji. This should significantly reduce layout costs, at least in browsers that support content-visibility (Safari has it in Tech Preview only).

Helps with #444 a lot, although there is probably more work we can do.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 7, 2024 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

benchmark-total

  • emoji-picker-element-change-tab: unsure 🔍 -4% - +3% (-1.57ms - +1.20ms)
    this-change vs tip-of-tree
  • emoji-picker-element-database-interactions: unsure 🔍 -2% - +8% (-2.34ms - +8.73ms)
    this-change vs tip-of-tree
  • emoji-picker-element-first-load: unsure 🔍 -6% - +5% (-3.33ms - +2.43ms)
    this-change vs tip-of-tree
  • emoji-picker-element-many-custom-emoji: faster ✔ 14% - 17% (38.03ms - 47.44ms)
    this-change vs tip-of-tree
  • emoji-picker-element-search: unsure 🔍 -4% - +5% (-2.05ms - +2.16ms)
    this-change vs tip-of-tree
  • emoji-picker-element-second-load: unsure 🔍 -0% - +7% (-0.08ms - +3.20ms)
    this-change vs tip-of-tree

Results

emoji-picker-element-change-tab
VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
this-change
33.74ms - 35.61ms-unsure 🔍
-4% - +3%
-1.57ms - +1.20ms
tip-of-tree
tip-of-tree		33.84ms - 35.88msunsure 🔍
-3% - +5%
-1.20ms - +1.57ms		-
emoji-picker-element-database-interactions
VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
this-change
109.29ms - 117.89ms-unsure 🔍
-2% - +8%
-2.34ms - +8.73ms
tip-of-tree
tip-of-tree		106.91ms - 113.88msunsure 🔍
-8% - +2%
-8.73ms - +2.34ms		-
emoji-picker-element-first-load
VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
this-change
51.05ms - 54.71ms-unsure 🔍
-6% - +5%
-3.33ms - +2.43ms
tip-of-tree
tip-of-tree		51.10ms - 55.56msunsure 🔍
-5% - +6%
-2.43ms - +3.33ms		-
emoji-picker-element-many-custom-emoji
VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
this-change
223.88ms - 230.11ms-faster ✔
14% - 17%
38.03ms - 47.44ms
tip-of-tree
tip-of-tree		266.20ms - 273.26msslower ❌
17% - 21%
38.03ms - 47.44ms		-
emoji-picker-element-search
VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
this-change
44.22ms - 47.32ms-unsure 🔍
-4% - +5%
-2.05ms - +2.16ms
tip-of-tree
tip-of-tree		44.29ms - 47.14msunsure 🔍
-5% - +4%
-2.16ms - +2.05ms		-
emoji-picker-element-second-load
VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
this-change
43.74ms - 46.13ms-unsure 🔍
-0% - +7%
-0.08ms - +3.20ms
tip-of-tree
tip-of-tree		42.25ms - 44.50msunsure 🔍
-7% - +0%
-3.20ms - +0.08ms		-

tachometer-reporter-action v2 for Benchmarks

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 7, 2024 •
edited
Loading

Size Change: +286 B (+0.79%)

Total Size: 36.7 kB

Filename Size Change
./bundle.js 36.7 kB +286 B (+0.79%)

compressed-size-action

@nolanlawson
Copy link
Owner Author

Hmm, this definitely helps in Chrome, but only has a small effect in Firefox and GNOME Canary. I guess it's a start, though – and way simpler than full-on virtualization.

@nolanlawson
Copy link
Owner Author

3-6% improvement in Firefox, yeah not as big.

┌─────────────┬──────────────────┐
│     Browser │ firefox-headless │
│             │ 130.0            │
├─────────────┼──────────────────┤
│ Sample size │ 200              │
└─────────────┴──────────────────┘

┌─────────────┬─────────────┬─────────────┬─────────────────────┬───────────────────┬───────────────────┐
│ Benchmark   │ Version     │ Bytes       │            Avg time │    vs this-change │    vs tip-of-tree │
│             │             │             │                     │                   │       tip-of-tree │
├─────────────┼─────────────┼─────────────┼─────────────────────┼───────────────────┼───────────────────┤
│ this-change │ <none>      │ 1070.70 KiB │ 398.64ms - 407.96ms │                   │            faster │
│             │             │             │                     │          -        │           3% - 6% │
│             │             │             │                     │                   │ 12.25ms - 25.93ms │
├─────────────┼─────────────┼─────────────┼─────────────────────┼───────────────────┼───────────────────┤
│ tip-of-tree │ tip-of-tree │ 1070.38 KiB │ 417.38ms - 427.40ms │            slower │                   │
│             │             │             │                     │           3% - 6% │          -        │
│             │             │             │                     │ 12.25ms - 25.93ms │                   │
└─────────────┴─────────────┴─────────────┴─────────────────────┴───────────────────┴───────────────────┘

@nolanlawson nolanlawson merged commit 9268c67 into master Sep 7, 2024
3 checks passed
This was referenced Sep 7, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@nolanlawson