[improvement] use of factory method to avoid OpenSpan.Builder allocations by wenhoujx · Pull Request #167 · palantir/tracing-java

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

wenhoujx · 2019-06-06T20:19:13Z

keep builder() method for back-compat.

Before this PR

flight recording of my java application shows OpenSpan$Builder has 3200 TLAB allocations amounts to 167.99 MB total.

After this PR

The static factory method of avoids Builder allocation.

- keep builder() method for back-compat

wenhoujx · 2019-06-06T20:19:35Z

wenhoujx · 2019-06-06T20:19:53Z

@carterkozak @schlosna

schlosna

👍 LGTM, think someone else will need to +1

Created #168 to remove some other allocations from hot paths when we're not observing that popped in similar profiles, especially for wrapped executors & high frequency endpoints at low sample rates.

schlosna · 2019-06-06T21:36:02Z

 // Avoid lambda allocation in hot paths
 if (prevState.isPresent()) {
- spanBuilder.parentSpanId(prevState.get().getSpanId());
+ span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(prevState.get().getSpanId()));


I realize this is existing and we're stuck with this API, but the Optional allocations on hot paths make me sad.

me sad too, do you think it's a good idea to create three methods to avoid the Optional wrapping?
static OpenSpan.of( .......other params...... Optional<String> parentSpanId) ; static OpenSpan.of( .......other params......String parentSpanId) ; static OpenSpan.of( .......other params.....) ;

I don't think its worth it as they'll still get created

iamdanfox · 2019-06-07T12:10:35Z

+ /**
+ * Use this factory method to avoid allocate {@link Builder} in hot path.
+ */
+ public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {


It's a bit of a shame that we have to make this public in order to access it within this library, as I really hope that no users will ever actually touch this class!

use of factory method to avoid OpenSpan.Builder allocations
2c99833
- keep builder() method for back-compat

wenhoujx requested a review from a team as a code owner June 6, 2019 20:19

schlosna mentioned this pull request Jun 6, 2019
[improvement] Avoid trace ID and span allocations when not observing #168
Closed

schlosna approved these changes Jun 6, 2019
View reviewed changes

schlosna mentioned this pull request Jun 7, 2019
Add JMH benchmarks #169
Closed

iamdanfox reviewed Jun 7, 2019
View reviewed changes

iamdanfox approved these changes Jun 7, 2019
View reviewed changes

iamdanfox added the merge when ready label Jun 7, 2019

bulldozer-botBot merged commit c1d5d74 into palantir:developJun 7, 2019

wenhoujx deleted the whou/path1/avoidBuilderAllocation branch June 7, 2019 15:16

schlosna mentioned this pull request Jun 14, 2019
[improvement] Add JMH benchmark project #176
Closed