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

Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint #172

Closed
wants to merge 2 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jul 20, 2020

The decision to unify the two SCALAR value types into a single data point message type is justified as follows:

  1. This reduces the size of Metric by one repeated field (in Go 24 bytes)
  2. This grows the corresponding data points slightly (in Go 8 bytes)
  3. The marginal memory cost of carrying both a int64 and double value in the same structure is minor, because that type already contains three other fields, one of them repeated. E.g., we have two 64 bit timestamps, one slice (3 x 64 bits), plus with the Google protoc-gen-go changes in v1.23.0 we have an additional pointer, uint32, and slice known as MessageState, SizeCache, and UnknownFields. Taken together, the Go ScalarDataPoint is less than 10% larger than the former Int64DataPoint.
  4. This can result in simpler implementation..
  5. Note that Add Exemplar support to Metrics proto #159 will replace the lost repeated field in this PR with a new repeated field of raw values, so this combined with Add Exemplar support to Metrics proto #159 will give a nearly break-even change.

@jmacd jmacd requested review from a team July 20, 2020 08:08
@jmacd
Copy link
Contributor Author

jmacd commented Jul 20, 2020

From #168 (comment)

The decision to unify the two SCALAR value types into a single data point message type is justified as follows: the marginal memory cost of carrying both a int64 and double value in the same structure is minor, because that type already contains three other fields, one of them repeated. E.g., we have two 64 bit timestamps, one slice (3 x 64 bits), plus with the Google protoc-gen-go v1.23.0 we have an additional pointer, uint32, and slice known as MessageState, SizeCache, and UnknownFields. Taken together, the Go ScalarDataPoint is less than 10% larger than the former Int64DataPoint.

This is a justification that it does not impact that much memory but not a justification why it benefits

The benefits are stated as (1), (4), and (5) in the description. It's (5) that I'm after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants