From 8b5f64881cbe9cf815890739594d01248e4a3f8f Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 25 Sep 2018 20:19:30 +0100 Subject: [PATCH] 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. --- Library/Homebrew/cache_store.rb | 81 +++-------------------- Library/Homebrew/linkage_cache_store.rb | 4 +- Library/Homebrew/test/cache_store_spec.rb | 6 +- 3 files changed, 12 insertions(+), 79 deletions(-) diff --git a/Library/Homebrew/cache_store.rb b/Library/Homebrew/cache_store.rb index 1bc48537e3..8f54c33ad4 100644 --- a/Library/Homebrew/cache_store.rb +++ b/Library/Homebrew/cache_store.rb @@ -1,6 +1,4 @@ -require "dbm" require "json" -require "timeout" # # `CacheStoreDatabase` acts as an interface to a persistent storage mechanism @@ -38,9 +36,10 @@ class CacheStoreDatabase db.delete(key) end - # Closes the underlying database (if it created and open). + # Closes the underlying database (if it is created and open). def close_if_open! - @db&.close + return unless @db + cache_path.atomic_write(JSON.dump(@db)) end # Returns `true` if the cache file has been created for the given `@type` @@ -52,55 +51,18 @@ class CacheStoreDatabase private - # The mode of any created files will be 0664 (that is, readable and writable - # by the owner and the group, and readable by everyone else). Files created - # will also be modified by the process' umask value at the time of creation: - # https://docs.oracle.com/cd/E17276_01/html/api_reference/C/envopen.html - DATABASE_MODE = 0664 - - # Spend 5 seconds trying to read the DBM file. If it takes longer than this it - # has likely hung or segfaulted. - DBM_TEST_READ_TIMEOUT = 5 - # Lazily loaded database in read/write mode. If this method is called, a # database file with be created in the `HOMEBREW_CACHE` with name # corresponding to the `@type` instance variable # - # @return [DBM] db + # @return [Hash] db def db - # DBM::WRCREAT: Creates the database if it does not already exist @db ||= begin - HOMEBREW_CACHE.mkpath - if created? - dbm_test_read_cmd = SystemCommand.new( - ENV["HOMEBREW_RUBY_PATH"], - args: [ - "-rdbm", - "-e", - "DBM.open('#{dbm_file_path}', #{DATABASE_MODE}, DBM::READER).values.size", - ], - print_stderr: false, - must_succeed: true, - ) - dbm_test_read_success = begin - Timeout.timeout(DBM_TEST_READ_TIMEOUT) do - dbm_test_read_cmd.run! - true - end - rescue ErrorDuringExecution, Timeout::Error - odebug "Failed to read #{dbm_file_path}!" - begin - Process.kill(:KILL, dbm_test_read_cmd.pid) - rescue Errno::ESRCH - # Process has already terminated. - nil - end - false - end - cache_path.delete unless dbm_test_read_success - end - DBM.open(dbm_file_path, DATABASE_MODE, DBM::WRCREAT) + JSON.parse(cache_path.read) if created? + rescue JSON::ParserError + nil end + @db ||= {} end # Creates a CacheStoreDatabase @@ -111,20 +73,12 @@ class CacheStoreDatabase @type = type end - # `DBM` appends `.db` file extension to the path provided, which is why it's - # not included - # - # @return [String] - def dbm_file_path - "#{HOMEBREW_CACHE}/#{@type}" - end - # The path where the database resides in the `HOMEBREW_CACHE` for the given # `@type` # # @return [String] def cache_path - Pathname("#{dbm_file_path}.db") + HOMEBREW_CACHE/"#{@type}.json" end end @@ -166,21 +120,4 @@ class CacheStore # @return [CacheStoreDatabase] attr_reader :database - - # DBM stores ruby objects as a ruby `String`. Hence, when fetching the data, - # to convert the ruby string back into a ruby `Hash`, the string is converted - # into a JSON compatible string in `ruby_hash_to_json_string`, where it may - # later be parsed by `JSON.parse` in the `json_string_to_ruby_hash` method - # - # @param [Hash] ruby `Hash` to be converted to `JSON` string - # @return [String] - def ruby_hash_to_json_string(hash) - hash.to_json - end - - # @param [String] `JSON` string to be converted to ruby `Hash` - # @return [Hash] - def json_string_to_ruby_hash(string) - JSON.parse(string) - end end diff --git a/Library/Homebrew/linkage_cache_store.rb b/Library/Homebrew/linkage_cache_store.rb index ad50ae7b41..d4b506e617 100644 --- a/Library/Homebrew/linkage_cache_store.rb +++ b/Library/Homebrew/linkage_cache_store.rb @@ -35,7 +35,7 @@ class LinkageCacheStore < CacheStore EOS end - database.set @keg_path, ruby_hash_to_json_string(hash_values) + database.set @keg_path, hash_values end # @param [Symbol] the type to fetch from the `LinkageCacheStore` @@ -68,6 +68,6 @@ class LinkageCacheStore < CacheStore keg_cache = database.get(@keg_path) return {} unless keg_cache - json_string_to_ruby_hash(keg_cache)[type.to_s] + keg_cache[type.to_s] end end diff --git a/Library/Homebrew/test/cache_store_spec.rb b/Library/Homebrew/test/cache_store_spec.rb index 0fdcc7d5ed..1c16ee65df 100644 --- a/Library/Homebrew/test/cache_store_spec.rb +++ b/Library/Homebrew/test/cache_store_spec.rb @@ -97,10 +97,6 @@ describe CacheStoreDatabase do describe "#close_if_open!" do context "database open" do - before do - subject.instance_variable_set(:@db, instance_double(DBM, close: nil)) - end - it "does not raise an error when `close` is called on the database" do expect { subject.close_if_open! }.not_to raise_error(NoMethodError) end @@ -118,7 +114,7 @@ describe CacheStoreDatabase do end describe "#created?" do - let(:cache_path) { Pathname("path/to/homebrew/cache/sample.db") } + let(:cache_path) { Pathname("path/to/homebrew/cache/sample.json") } before do allow(subject).to receive(:cache_path).and_return(cache_path)