-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
tree: Improve performance of tree.AsJSON #87968
Conversation
Full benchmark stat: |
|
1ca0ef9
to
10c662e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 2 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @miretskiy)
pkg/geo/bbox.go
line 46 at r2 (raw file):
// AppendFormat appends string representation of the CartesianBoundingBox // to the buffer, and returns modified buffer. func (b *CartesianBoundingBox) AppendFormat(buf []byte) []byte {
You might want to look into having this method take a *bytes.Buffer instead and then calling strconv.AppendFloat(buf.Bytes(),...
. Might be more trouble than it's worth though since you probably need to check capacity.
pkg/geo/bbox.go
line 49 at r2 (raw file):
// fmt.Sprintf with %f does not truncate leading zeroes, so use // AppendFloat instead. buf = append(buf, "BOX("...)
Nit: I think a constant performs better than a literal here?
pkg/util/strutil/util.go
line 24 at r2 (raw file):
} var scratch [16]byte
Maybe take this as an argument instead of allocating it here?
pkg/util/timetz/timetz.go
line 158 at r2 (raw file):
// AppendFormat appends TimeTZ to the buffer, and returns modified buffer. func (t *TimeTZ) AppendFormat(buf []byte) []byte { if len(buf) < 32 {
Use cap rather than len, I think? Or this also looks like another argument for taking a bytes.Buffer in these methods.
pkg/util/timetz/timetz.go
line 168 at r2 (raw file):
buf = append(buf, "24:00:00"...) } else { buf = tTime.AppendFormat(buf, "15:04:05.999999")
Looks like placeholder code here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB)
pkg/geo/bbox.go
line 46 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
You might want to look into having this method take a *bytes.Buffer instead and then calling
strconv.AppendFloat(buf.Bytes(),...
. Might be more trouble than it's worth though since you probably need to check capacity.
I tried various ways; in the end, lots of code could use bytes.Buffer, but lots also needs []byte (e.g. strconv); settled on the lowest denominator.
pkg/geo/bbox.go
line 49 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Nit: I think a constant performs better than a literal here?
Wdym? like const prefix = "BOX("
? Why would it be better?
pkg/util/strutil/util.go
line 24 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Maybe take this as an argument instead of allocating it here?
This is actually similar to how appendInt is implemented in time.Time (part of format); I initially
cribbed this method from there, but then realized that I can make it simpler (and more efficient) by using strconv directly (something time.time wants to avoid). At any rate, using scratch space on the stack is very common and very fast.
pkg/util/timetz/timetz.go
line 158 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Use cap rather than len, I think? Or this also looks like another argument for taking a bytes.Buffer in these methods.
I removed this code.
pkg/util/timetz/timetz.go
line 168 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Looks like placeholder code here
No, it's no placeholder -- 24:00:00 is what master version returns -- and the same thing I return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @miretskiy)
pkg/util/timetz/timetz.go
line 168 at r2 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
No, it's no placeholder -- 24:00:00 is what master version returns -- and the same thing I return.
Sorry I meant to leave this comment on timeComponent := tTime.Format("15:04:05.999999")
|
Withdrawn, I saw that done in fmt and thought it was for performance but I think I was wrong and it makes no difference. |
CI failed due to flaky test being fixed in #87997 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvements!
Reviewed 1 of 2 files at r1, 15 of 15 files at r3, 13 of 14 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @miretskiy)
-- commits
line 2 at r3:
nit: s/benchmar./benchmark/
pkg/geo/bbox.go
line 47 at r5 (raw file):
// to the buffer, and returns modified buffer. func (b *CartesianBoundingBox) AppendFormat(buf []byte) []byte { // fmt.Sprintf with %f does not truncate leading zeroes, so use
nit: we no longer use fmt.Sprintf
so the comment should be updated.
pkg/sql/sem/tree/datum.go
line 1786 at r5 (raw file):
} var buf [uuid.RFC4122StrSize]byte
nit: should we reuse the newly-introduced ctx.scratch
here? It should be a stack allocation so it might not matter much.
pkg/sql/sem/tree/datum.go
line 3738 at r5 (raw file):
// with certain JSON consumers, so we'll use an alternate formatting // path here to maintain consistency with PostgreSQL. return json.FromString(formatTime(t.UTC(), time.RFC3339Nano)), nil
Previously we were using loc
, so I think there is something off here.
pkg/sql/sem/tree/datum.go
line 3759 at r5 (raw file):
} // formatTime formats time with specified layout.
nit: could you please a TODO(yuzefovich)
to consider using this function in more spots?
pkg/util/strutil/util.go
line 16 at r5 (raw file):
// AppendInt appends the decimal form of x to b and returns the result. // If the decimal form is shorter than width, the result is padded with leading 0's.
nit: mention what happens if the decimal form is longer than width
.
pkg/util/strutil/util.go
line 20 at r5 (raw file):
if x < 0 { width-- x *= -1
nit: it might be faster to do x = -x
.
pkg/util/timeofday/time_of_day.go
line 58 at r5 (raw file):
} // AppendFormat appends this TimeOfDay format to the specified buffer
nit: missing period.
pkg/sql/sem/tree/json_test.go
line 26 at r3 (raw file):
func BenchmarkAsJSON(b *testing.B) { rng := randutil.NewTestRandWithSeed(-4365865412074131521)
nit: is the usage of a fixed seed here to reduce the variance? It probably deserves a quick comment.
pkg/sql/sem/tree/json_test.go
line 78 at r3 (raw file):
// AsJson(Geo) -> MarshalGeo -> go JSON bytes -> ParseJSON -> Go native -> json.JSON // Benchmarking this generates too much noise. // TODO: fix this.
nit: TODOs should have a github username attached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @yuzefovich)
pkg/geo/bbox.go
line 47 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: we no longer use
fmt.Sprintf
so the comment should be updated.
I think the comment used sense; I think it was intended to warn to not use Sprintf;
But I guess it doesn't matter -- removed.
pkg/sql/sem/tree/datum.go
line 1786 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: should we reuse the newly-introduced
ctx.scratch
here? It should be a stack allocation so it might not matter much.
I guess we could... As long as scratch > 36 bytes (which it is.. I added a comment there to make sure it's not reduced below).
pkg/sql/sem/tree/datum.go
line 3738 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Previously we were using
loc
, so I think there is something off here.
Good catch...
pkg/sql/sem/tree/datum.go
line 3759 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: could you please a
TODO(yuzefovich)
to consider using this function in more spots?
Ack.
pkg/util/strutil/util.go
line 16 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: mention what happens if the decimal form is longer than
width
.
Done; also added a test.
pkg/util/strutil/util.go
line 20 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: it might be faster to do
x = -x
.
Done.
pkg/util/timetz/timetz.go
line 168 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Sorry I meant to leave this comment on
timeComponent := tTime.Format("15:04:05.999999")
Done.
pkg/sql/sem/tree/json_test.go
line 26 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: is the usage of a fixed seed here to reduce the variance? It probably deserves a quick comment.
Yeah; Not just reduce variance but have reproducible results so that if you make change,
you can rerun against the same types and exactly the same datum stream to see the effect.
Comment added.
pkg/sql/sem/tree/json_test.go
line 78 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: TODOs should have a github username attached.
Sure, I'll volunteer :) Eventually.
For now, it was a bit too big of a lift.
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Add a micro benchmark for `tree.AsJSON` method. Release note: None Release justification: test only change
Improve performance of `tree.AsJSON` method. These improvements are important for any query that produces large number of JSON objects, as well as to changefeeds, which rely on this function when producing JSON encoded feed. Most of the changes revolved around modifying underlying types (s.a. date/timestamp types, box2d, etc) to favor using functions that append to bytes buffer, instead of relying on slower functions, such as `fmt.Sprintf`. The conversion performance improved around 5-10% for most of the types, and as high as 50% for time types: ``` Benchmark old t/op new t/op delta AsJSON/box2d-10 578ns ± 3% 414ns ± 2% -28.49% (p=0.000 n=10+9) AsJSON/box2d[]-10 1.64µs ± 3% 1.19µs ± 4% -27.14% (p=0.000 n=10+10) AsJSON/time-10 232ns ± 2% 103ns ± 1% -55.61% (p=0.000 n=10+10) AsJSON/time[]-10 687ns ± 4% 342ns ± 4% -50.17% (p=0.000 n=10+10) ``` Note: Some types in the local benchmark show slight slow down in speed. No changes were made in those types, and in general, the encoding speed of these types might be too fast to reliable detect changes: ``` Benchmark old t/op new t/op delta AsJSON/bool[]-10 65.9ns ± 1% 67.7ns ± 2% +2.79% (p=0.001 n=8+9) ``` The emphasis was also placed on reducing allocations. By relying more heavily on a pooled FmtCtx, which contains bytes buffer, some conversions resulted in amortized elimination of allocations (time): ``` Benchmark old B/op new t/op delta AsJSON/timestamp-10 42.1B ± 3% 0.0B -100.00% (p=0.000 n=10+10) AsJSON/timestamp[]-10 174B ± 4% 60B ± 1% -65.75% (p=0.000 n=10+10) ``` Release Note: None Release Justification: performance improvement
Bors r+ |
Build succeeded: |
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
88064: changefeedccl: Improve JSON encoder performance r=honoredb a=miretskiy Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in #87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Rewrite JSON encoder to improve its performance. Prior to this change JSON encoder was very inefficient. This inefficiency had multiple underlying reasons: * New Go map objects were constructed for each event. * Underlying json conversion functions had inefficiencies (tracked in cockroachdb#87968) * Conversion of Go maps to JSON incurs the cost of sorting the keys -- for each row. Sorting, particularly when rows are wide, has significant cost. * Each conversion to JSON allocated new array builder (to encode keys) and new object builder; that too has cost. * Underlying code structure, while attempting to reuse code when constructing different "envelope" formats, cause the code to be more inefficient. This PR addresses all of the above. In particular, since a schema version for the table is guaranteeed to have the same set of primary key and value columns, we can construct JSON builders once. The expensive sort operation can be performed once per version; builders can be memoized and cached. The performance impact is significant: * Key encoding speed up is 5-30%, depending on the number of primary keys. * Value encoding 30% - 60% faster (slowest being "wrapped" envelope with diff -- which effectively encodes 2x values) * Byte allocations per row reduces by over 70%, with the number of allocations reduced similarly. Release note (enterprise change): Changefeed JSON encoder performance improved by 50%. Release justification: performance improvement
Improve performance of
tree.AsJSON
method.These improvements are important for any query that produces
large number of JSON objects, as well as to changefeeds, which
rely on this function when producing JSON encoded feed.
Most of the changes revolved around modifying underlying types
(s.a. date/timestamp types, box2d, etc) to favor using functions
that append to bytes buffer, instead of relying on slower
functions, such as
fmt.Sprintf
. The conversionperformance improved around 5-10% for most of the types, and
as high as 50% for time types:
Note: Some types in the local benchmark show slight slow down in speed.
No changes were made in those types, and in general, the encoding speed
of these types might be too fast to reliable detect changes:
The emphasis was also placed on reducing allocations.
By relying more heavily on a pooled FmtCtx, which contains
bytes buffer, some conversions resulted in amortized
elimination of allocations (time):
Release Note: None
Release Justification: performance improvement