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

Linux and *BSD Support #43

Merged
merged 2 commits into from
May 25, 2017
Merged

Linux and *BSD Support #43

merged 2 commits into from
May 25, 2017

Conversation

burke
Copy link
Member

@burke burke commented May 24, 2017

This PR migrates from xattr as a cache backend to a simple directory of files.

The cache file naming scheme is A/B[0..1]/B[2..-1], where:

  • A is the cache_dir provided on Bootsnap.setup;
  • B is a hex-encoded FNV1a-64 hash of the fully-expanded path of the file being cached.

The cache key is similar to the previous iteration with the following significant changes:

  • Added original file size to cache key (size) as added insurance against hash collisions, etc. Probably not necessary, but it's effectively free.
  • Padded cache key to 64 bytes in order to be able to change it in the future without modifying its length.

As a bonus, this seems to be (very) marginally faster than master.


This unfortunately ends up being a fairly brutal diff to review since it's a near-complete rewrite of bootsnap.c, so if I've tagged you for review, I won't hold it against you if you only take a cursory look. I think the C got a little bit easier to read than it was before but it's still a bit of a project.

/cc @SamSaffron

@burke burke force-pushed the linux branch 7 times, most recently from 3d868e9 to 41c7b42 Compare May 24, 2017 20:32
@burke burke force-pushed the linux branch 5 times, most recently from 472f0d6 to 606abd4 Compare May 24, 2017 21:46
@burke burke changed the title 🚧 Linux Support 🚧 Linux and *BSD Support May 25, 2017
@burke burke force-pushed the linux branch 2 times, most recently from 9f60267 to c417ca7 Compare May 25, 2017 01:46
@burke burke changed the title 🚧 Linux and *BSD Support Linux and *BSD Support May 25, 2017
@burke
Copy link
Member Author

burke commented May 25, 2017

¯\_(ツ)_/¯ I'm just gonna merge and encourage people to try out 0.3.0.pre.

@burke burke merged commit 5a3ffd8 into master May 25, 2017
@burke burke deleted the linux branch May 25, 2017 18:11
@SamSaffron
Copy link
Contributor

totally fine with me, I am meaning to do some perf measurments on boot impact, will let you know once it is done.

@SamSaffron
Copy link
Contributor

SamSaffron commented May 26, 2017

This is looking awesome performance wise @burke

Baseline on Linux:

sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  3.06s user 0.73s system 98% cpu 3.857 total
sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  3.07s user 0.72s system 98% cpu 3.862 total

0.2.14 on Linux:

bin/rails r 'I18n.t "test"'  2.81s user 0.29s system 98% cpu 3.140 total
sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  2.77s user 0.27s system 98% cpu 3.074 total
sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  2.85s user 0.30s system 98% cpu 3.185 total

0.3.0.pre no ISEQ/YAML cache yet:

sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  2.52s user 0.35s system 98% cpu 2.900 total
sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  2.54s user 0.33s system 98% cpu 2.914 total
sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  2.57s user 0.34s system 98% cpu 2.950 total

0.3.0.pre ISEQ cache

sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  1.88s user 0.59s system 97% cpu 2.519 total
sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  1.77s user 0.60s system 97% cpu 2.416 total

0.3.0.pre ISEQ + YAML cache

sam@ubuntu discourse % time bin/rails r 'I18n.t "test"'
bin/rails r 'I18n.t "test"'  1.62s user 0.63s system 97% cpu 2.297 total

Excellent work. This is faster than old version under ALL conditions for us.

SamSaffron added a commit to discourse/discourse that referenced this pull request May 26, 2017
@zmoazeni
Copy link

Works here too on ubuntu 16.04.

Cuts off about 2.5 seconds from the bootup on our codebase. Thanks for the hard work!!

@SamSaffron
Copy link
Contributor

@zmoazeni what was the absolute time prior to bootsnap?

@zmoazeni
Copy link

@SamSaffron re-running the same command you posted above here's before and after:

# without bootsnap
∴ time bin/rails r 'I18n.t "test"'

real	0m4.188s
user	0m3.068s
sys	0m1.036s

# 0.3.0.pre ISEQ + YAML cache
∴ time bin/rails r 'I18n.t "test"'

real	0m1.820s
user	0m1.660s
sys	0m0.140s

@SamSaffron
Copy link
Contributor

wow... that is huge!

@zmoazeni
Copy link

Here's another data point. The numbers above were from my beefy development machine.

Here's the numbers on the same codebase on a Dell XPS 13 also running Ubuntu 16.04 (my travel machine):

# without bootsnap
∴ time bin/rails r 'I18n.t "test"'

real	0m7.020s
user	0m4.744s
sys	0m2.196s

# 0.3.0.pre ISEQ + YAML cache
∴ time bin/rails r 'I18n.t "test"'

real	0m2.507s
user	0m2.244s
sys	0m0.232s

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.

3 participants