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

Refactor in where obtain hash keys by using common function #887

Merged
merged 1 commit into from
Jul 2, 2023

Conversation

Watson1978
Copy link
Collaborator

Refactor in where obtain hash keys by using common function. However, it might have the margin of error for the overhead of calling oj_calc_hash_key().

before after result
Oj.dump 449.521k 447.738k 0.996x

Environment

  • Linux
    • Manjaro Linux x86_64
    • Kernel: 6.3.5-2-MANJARO
    • AMD Ryzen 7 5800H
    • gcc version 13.1.1
    • Ruby 3.2.2

Before

Warming up --------------------------------------
             Oj.load    44.738k i/100ms
Calculating -------------------------------------
             Oj.load    449.521k (± 0.9%) i/s -      6.755M in  15.029388s

After

Warming up --------------------------------------
             Oj.load    44.117k i/100ms
Calculating -------------------------------------
             Oj.load    447.738k (± 0.3%) i/s -      6.750M in  15.075672s

Test code

require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem 'benchmark-ips'
  gem 'oj'
end

json =<<-EOF
{
  "$id": "https://example.com/person.schema.json",
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "title": "Person",
  "type": "object",
  "properties": {
    "firstName": {
      "type": "string",
      "description": "The person's first name."
    },
    "lastName": {
      "type": "string",
      "description": "The person's last name."
    },
    "age": {
      "description": "Age in years which must be equal to or greater than zero.",
      "type": "integer",
      "minimum": 0
    }
  }
}
EOF

Benchmark.ips do |x|
  x.warmup = 5
  x.time = 15

  x.report('Oj.load') { Oj.load(json, mode: :compat) }
end

ext/oj/compat.c Outdated
@@ -25,20 +25,8 @@ static void hash_set_cstr(ParseInfo pi, Val kval, const char *str, size_t len, c
parent->clen = len;
} else {
volatile VALUE rstr = oj_cstr_to_value(str, len, (size_t)pi->options.cache_str);
rkey = oj_calc_hash_key(pi, kval);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A further optimization would be to remove line 19 then line 21 becomes:
if (Qundef == val->key_val && Yes == pi->options.create_ok && NULL != pi->options.create_id &&
and the new line 28 becomes volatile VALUE rkey = oj_calc_hash_key(pi, kval);.

Since oj_calc_hash_key already gets rkey from the parent the suggested change avoids the assignment and stack variable us in the first block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review. I have fixed the codes to follow the suggestion.

Refactor in where obtain hash keys by using common function.
However, it might have the margin of error for the overhead of calling oj_calc_hash_key().

−       | before   | after    | result
--       | --       | --       | --
Oj.dump  | 449.521k | 447.738k | 0.996x

### Environment
- Linux
  - Manjaro Linux x86_64
  - Kernel: 6.3.5-2-MANJARO
  - AMD Ryzen 7 5800H
  - gcc version 13.1.1
  - Ruby 3.2.2

### Before
```
Warming up --------------------------------------
             Oj.load    44.738k i/100ms
Calculating -------------------------------------
             Oj.load    449.521k (± 0.9%) i/s -      6.755M in  15.029388s
```

### After
```
Warming up --------------------------------------
             Oj.load    44.117k i/100ms
Calculating -------------------------------------
             Oj.load    447.738k (± 0.3%) i/s -      6.750M in  15.075672s
```

### Test code
```ruby
require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem 'benchmark-ips'
  gem 'oj'
end

json =<<-EOF
{
  "$id": "https://example.com/person.schema.json",
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "title": "Person",
  "type": "object",
  "properties": {
    "firstName": {
      "type": "string",
      "description": "The person's first name."
    },
    "lastName": {
      "type": "string",
      "description": "The person's last name."
    },
    "age": {
      "description": "Age in years which must be equal to or greater than zero.",
      "type": "integer",
      "minimum": 0
    }
  }
}
EOF

Benchmark.ips do |x|
  x.warmup = 5
  x.time = 15

  x.report('Oj.load') { Oj.load(json, mode: :compat) }
end
```
@ohler55 ohler55 merged commit 0c27fa1 into ohler55:develop Jul 2, 2023
36 checks passed
@Watson1978 Watson1978 deleted the hash-key branch July 3, 2023 01:06
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.

None yet

2 participants