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

Reduce memory footprint and GC pressure #446

Merged
merged 12 commits into from
Jun 11, 2018

Conversation

pawelchcki
Copy link
Contributor

@pawelchcki pawelchcki commented Jun 7, 2018

This PR tries to optimize memory used by enabled integrations.

Each traced operation creates a span to store the data about it. Each created span has associated cost, both related to creation time and memory footprint of Span object.

Inspecting code and looking at measurements produced by derailed gem helped identify spots of unnecessary temporary object creation.

This PR reduces the incidence of temporary object creation and by switching storing of timestamps to much smaller Floats (before they were full Time objects, with some temporary objects created in between.

It reduces the maximum memory used by test application by up to 50%.

Examples of changes:

  • each configuration access triggered creation of temporary Proxy objects, now they are cached
  • quite a few strings were allocated with every span creation, .freeze-ing those solved that problem (at least for rubies 2.2+)
  • Utils.truncate was used to truncate Strings, however it did so by creating around 3 temporary strings each with a copy of truncated string. New #truncate! method uses the fact that all those Strings can be safely modified to truncate the object without any temporary object creation.
  • Time.now.utc requires 2 Time objects to be created. Process.clock_gettime(Process::CLOCK_REALTIME) only creates one Float which has much negligible footprint compared to Time.

@pawelchcki pawelchcki added the do-not-merge/WIP Not ready for merge label Jun 7, 2018
@pawelchcki pawelchcki self-assigned this Jun 7, 2018
@delner
Copy link
Contributor

delner commented Jun 7, 2018

Could you explain some of the issues you suspect are taking up the most memory and how you're solving them? It would be great to have some context to consider these changes in.

@pawelchcki pawelchcki force-pushed the bugfix/reduce_memory_footprint_and_gc_pressure branch from 56cb6ff to 69039b3 Compare June 8, 2018 13:36
@pawelchcki pawelchcki added this to the 0.12.1 milestone Jun 8, 2018
@pawelchcki pawelchcki added bug Involves a bug core Involves Datadog core libraries integrations Involves tracing integrations and removed integrations Involves tracing integrations do-not-merge/WIP Not ready for merge labels Jun 8, 2018
@pawelchcki pawelchcki requested a review from delner June 11, 2018 08:29
@pawelchcki pawelchcki removed their assignment Jun 11, 2018
@cbliard
Copy link
Contributor

cbliard commented Jun 11, 2018

Maybe that's not the point in this pull request, but shouldn't Process::CLOCK_MONOTONIC be preferred over Process::CLOCK_REALTIME?

Processes like ntp can change the time, and the value returned by Process.clock_gettime(Process::CLOCK_REALTIME) can be affected, distorting the computed elapsed time. This is solved by using Process.clock_gettime(Process::CLOCK_MONOTONIC) which measures the elapsed time from some unspecified starting point.

This article goes in depth about this topic.

@delner
Copy link
Contributor

delner commented Jun 11, 2018

@cbliard That's a good suggestion. I think we might address any changes to handling time in a different pull request altogether, given the complexity it introduces, and the impact it could have. We do already have #424 open as an implementation of Process::CLOCK_MONOTONIC, but that requires some more work itself. Perhaps that's the best place to discuss this kind of change.

@pawelchcki pawelchcki force-pushed the bugfix/reduce_memory_footprint_and_gc_pressure branch from f3f2cdc to b312af5 Compare June 11, 2018 17:33
@pawelchcki pawelchcki force-pushed the bugfix/reduce_memory_footprint_and_gc_pressure branch from b312af5 to 2161915 Compare June 11, 2018 17:37
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

This all looks pretty good to me! 👍

@delner delner merged commit 6c12ab0 into master Jun 11, 2018
@delner delner deleted the bugfix/reduce_memory_footprint_and_gc_pressure branch June 20, 2018 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants