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

Blueprinter: Render Performance Optimisations #289

Closed
wants to merge 10 commits into from
Closed

Blueprinter: Render Performance Optimisations #289

wants to merge 10 commits into from

Conversation

midhun-thimmapuram
Copy link

@midhun-thimmapuram midhun-thimmapuram commented May 25, 2022

Benchmark comparison:

Output on Ruby 2.6.9, MacBook Pro (15-inch, 2018, 2.2 GHz 6-Core Intel Core i7)

benchmark code

blueprinter 0.25.3 (current master)

Rehearsal -----------------------------------------------
blueprinter   0.362873   0.004129   0.367002 (  0.367198)
-------------------------------------- total: 0.367002sec

                  user     system      total        real
blueprinter   0.337027   0.001376   0.338403 (  0.338711)
Warming up --------------------------------------
         blueprinter     1.000  i/100ms
Calculating -------------------------------------
         blueprinter      2.724  (± 2.2%) i/s -     28.000  in  10.326712s
                   with 95.0% confidence
Calculating -------------------------------------
         blueprinter    63.991M memsize (     0.000  retained)
                       480.353k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

With current PR optimisations Run - 1

Rehearsal -----------------------------------------------
blueprinter   0.203004   0.004435   0.207439 (  0.207799)
-------------------------------------- total: 0.207439sec

                  user     system      total        real
blueprinter   0.192406   0.000247   0.192653 (  0.192837)
Warming up --------------------------------------
         blueprinter     1.000  i/100ms
Calculating -------------------------------------
         blueprinter      4.718  (± 3.3%) i/s -     47.000  in  10.099948s
                   with 95.0% confidence
Calculating -------------------------------------
         blueprinter    16.860M memsize (     0.000  retained)
                        80.139k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

With current PR optimisations Run - 2

Rehearsal -----------------------------------------------
blueprinter   0.219407   0.005306   0.224713 (  0.227365)
-------------------------------------- total: 0.224713sec

                  user     system      total        real
blueprinter   0.208257   0.000257   0.208514 (  0.208746)
Warming up --------------------------------------
         blueprinter     1.000  i/100ms
Calculating -------------------------------------
         blueprinter      4.405  (± 3.8%) i/s -     44.000  in  10.165494s
                   with 95.0% confidence
Calculating -------------------------------------
         blueprinter    16.799M memsize (     0.000  retained)
                        79.889k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

@midhun-thimmapuram midhun-thimmapuram marked this pull request as ready for review May 26, 2022 04:55
@midhun-thimmapuram midhun-thimmapuram changed the base branch from master to v0.19.0 May 26, 2022 11:44
@midhun-thimmapuram midhun-thimmapuram changed the base branch from v0.19.0 to master May 26, 2022 11:47
@midhun-thimmapuram
Copy link
Author

@skryukov What's next for PR to be merged?

@skryukov
Copy link

skryukov commented Jun 9, 2022

We should wait for maintainers, I'm just an outsider 😅

@mcclayton
Copy link
Contributor

Hi @midhun-thimmapuram, I received your message, but I'm no longer a maintainer / part of the Procore org. and therefore am unable to provide an approving review that will enable you to merge.
See: #288 (comment)

Copy link

@terrence2 terrence2 left a comment

Choose a reason for hiding this comment

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

I'm largely unfamiliar with the context, but there appears to be two unrelated changes here.

The performance optimization is coming from the memoization of field sorting in fields_for, which is a nice, simple, and clean improvement.

There's also a mild refactor of date time formatting that appears to not change the operations that occur per format, but does clarify the code somewhat, so is a nice patch as well.

Given that these are unrelated, I'd much prefer to have them land as separate patches, but not enough that I'd block getting this landed at all, given how long it's been open.

@midhun-thimmapuram
Copy link
Author

terrence2

Thanks for the approval, I can create another PR moving the date time formatting changes there if it is required so that we can have them as separate patches.

@midhun-thimmapuram midhun-thimmapuram closed this by deleting the head repository Jan 31, 2023
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.

4 participants