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

cache_store: use JSON. #4984

Merged
merged 1 commit into from Sep 26, 2018

Conversation

Projects
None yet
3 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Sep 25, 2018

After all our recent troubles with DBM I figured I'd benchmark the performance of DBM vs. JSON. At read time (what we care more about) the performance is pretty much identical and JSON is only 1.5x slower at write time. This seems worth it for the reliability increases to avoid messing with unreliable native code.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

cache_store: use JSON.
After all our recent troubles with DBM I figured I'd benchmark the
performance of DBM vs. JSON. At read time (what we care more about) the
performance is pretty much identical and JSON is only 1.5x slower at
write time. This seems worth it for the reliability increases to avoid
messing with unreliable native code.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:json-cache-store branch from ca66b57 to 8b5f648 Sep 25, 2018

@colindean
Copy link
Member

colindean left a comment

This seems like a good use of JSON since the user won't ever be editing it.

@MikeMcQuaid MikeMcQuaid merged commit 02cc0be into Homebrew:master Sep 26, 2018

3 checks passed

codecov/patch 87.5% of diff hit (target 71.24%)
Details
codecov/project 71.25% (+0.01%) compared to c35ade1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:json-cache-store branch Sep 26, 2018

@wafflebot wafflebot bot removed the in progress label Sep 26, 2018

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Sep 26, 2018

I think this might have caused Homebrew/homebrew-core#32430

Namely, user is running brew doctor and gets:

Error: 419: unexpected token at '"/usr/lib/libS/usr/local/Cellar/autoconf/2.69'
Please report this bug:
  https://docs.brew.sh/Troubleshooting
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/json/common.rb:156:in `parse'
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/json/common.rb:156:in `parse'
/usr/local/Homebrew/Library/Homebrew/cache_store.rb:185:in `json_string_to_ruby_hash'
/usr/local/Homebrew/Library/Homebrew/linkage_cache_store.rb:71:in `fetch_hash_values'
/usr/local/Homebrew/Library/Homebrew/linkage_cache_store.rb:53:in `fetch_type'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:84:in `check_dylibs'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:25:in `initialize'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1790:in `new'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1790:in `block in undeclared_runtime_dependencies'
/usr/local/Homebrew/Library/Homebrew/cache_store.rb:17:in `use'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1789:in `undeclared_runtime_dependencies'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1562:in `runtime_dependencies'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1568:in `runtime_formula_dependencies'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1584:in `missing_dependencies'
/usr/local/Homebrew/Library/Homebrew/diagnostic.rb:13:in `block in missing_deps'
/usr/local/Homebrew/Library/Homebrew/diagnostic.rb:12:in `each'
/usr/local/Homebrew/Library/Homebrew/diagnostic.rb:12:in `missing_deps'
/usr/local/Homebrew/Library/Homebrew/diagnostic.rb:652:in `check_missing_deps'
/usr/local/Homebrew/Library/Homebrew/cmd/doctor.rb:46:in `block in doctor'
/usr/local/Homebrew/Library/Homebrew/cmd/doctor.rb:38:in `each'
/usr/local/Homebrew/Library/Homebrew/cmd/doctor.rb:38:in `doctor'
/usr/local/Homebrew/Library/Homebrew/brew.rb:91:in `<main>'

Edit: I am wrong (again), because the user doesn't have that commit in the brew history. Sorry :(

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 26, 2018

@fxcoudert no worries. Get them to delete their linkage.db.

@lock lock bot added the outdated label Oct 26, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.