cloudfoundry / bosh-azure-cpi-release

BOSH Azure CPI
Apache License 2.0
63 stars 88 forks source link

Improve JSON parsing speed by using Oj or FastJsonParser #667

Closed fearoffish closed 1 year ago

fearoffish commented 2 years ago

At SAP, we have a huge amount of vms and in turn, network interfaces. We're seeing a significant wait time when rebuilding environments (which we do regularly) and we see a good amount of time wasted is in the JSON parsing of REST API calls. As an example, we see 3 calls to the API each returning 800Kb of JSON containing network interfaces which takes the BOSH process 12 seconds to parse (and call).

I propose a simple addition to the azure cpi, of the Oj rubygem which would improve that by roughly ⅓ .

I've run a benchmark of the JSON parsing in the get_resources_by_url method, using an 800Kb JSON file:

Warming up --------------------------------------
                json     1.000  i/100ms
                  oj     3.000  i/100ms
                fast     4.000  i/100ms
Calculating -------------------------------------
                json     18.872  (±21.2%) i/s -    179.000  in  10.030650s
                  oj     29.329  (±10.2%) i/s -    291.000  in  10.003598s
                fast     37.008  (±10.8%) i/s -    368.000  in  10.051131s

Comparison:
                fast:       37.0 i/s
                  oj:       29.3 i/s - 1.26x  (± 0.00) slower
                json:       18.9 i/s - 1.96x  (± 0.00) slower

In the benchmark I experiemented with the Oj gem and FastJsonParser. The largest increase in speed is from Oj, with a minor bump again if we use FastJsonParser. Honestly, I'm only proposing the Oj inclusion, it's much simpler and requires much less work.

I can create a PR if the community/maintainers would accept the PR, as I've created it already on an internal fork to play with. Real world results will be available soon too, once we push it to live and record the changes.

To be clear, the PR will do the following:

What are your thoughts?

mvach commented 2 years ago

Hi,

inside SAP we are already testing Fastjsonparser for azure_cpi. So I'll take here.

rkoster commented 2 years ago

This seems like a good idea. It should be relatively cheap to implement this behind a global cpi flag. Which would further derisk this feature.

fearoffish commented 2 years ago

We've been using this since July at SAP and we are seeing no problems. @rkoster Would we still need the CPI flag, or shall I just create a PR without?

rkoster commented 2 years ago

Just create a PR without it and see what the approvers think.

mvach commented 2 years ago

@fearoffish while preparing the required PR (https://github.com/mvach/bosh-azure-cpi-release/tree/issue-667), I noticed that we missed to replace 2 JSON.parse invocations.

https://github.com/cloudfoundry/bosh-azure-cpi-release/blob/2aeb7bd3ed5e47d10a4fad22f49f142ff05ca14b/src/bosh_azure_cpi/lib/cloud/azure/stemcell/light_stemcell_manager.rb#L57 and https://github.com/cloudfoundry/bosh-azure-cpi-release/blob/2aeb7bd3ed5e47d10a4fad22f49f142ff05ca14b/docs/advanced/configure-cf-external-databases-using-azure-mysql-postgres-service/migrate-mysql-databases/migrate-mysql-databases.rb#L126

While the first one is trivial, the second one isn't just a replacement and would require some time. But since it is a script in the docs from 2018 and for mysql I'm unsure if we have to send the effort at all.

Let's have a short discussion about that.

cunnie commented 1 year ago

Hey @mvach , could you please run bin/vendor_gems next time you bump a gem? Otherwise our CI comes to a screeching halt. Thanks.