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.

Sign up for GitHub

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

Jump to bottom

kv: don't heap allocate transaction deadline timestamp #74224

Closed
nvanbenschoten opened this issue Dec 22, 2021 · 2 comments · Fixed by #85408
Closed

kv: don't heap allocate transaction deadline timestamp #74224

nvanbenschoten opened this issue Dec 22, 2021 · 2 comments · Fixed by #85408
Assignees
@nvanbenschoten
Labels
A-kv-transactions Relating to MVCC and the transactional model. C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-kv KV Team

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Dec 22, 2021 •
edited by cockroach-jira-scripts
Loading

NOTE: this needs to wait for the v22.2 release cycle.

We currently heap allocate when maintaining a kv.Txn's deadline here. The use of *hlc.Timestamp to represent an optional timestamp is mostly an anti-pattern at this point, now that the type has an IsEmpty() method that corresponds to the type's zero value.

In a run of sysbench's oltp_point_select workload, this was the 9th most frequent source of heap allocations, accounting for over 1% of total allocations.

----------------------------------------------------------+-------------
      flat  flat%   sum%        cum   cum%   calls calls% + context 	 	 
----------------------------------------------------------+-------------
...
----------------------------------------------------------+-------------
                                          63603658   100% |   github.com/cockroachdb/cockroach/pkg/sql/catalog/descs.(*leasedDescriptors).maybeUpdateDeadline /go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/leased_descriptors.go:217
  63603658  1.06% 13.90%   63603658  1.06%                | github.com/cockroachdb/cockroach/pkg/kv.(*Txn).UpdateDeadline /go/src/github.com/cockroachdb/cockroach/pkg/kv/txn.go:784
----------------------------------------------------------+-------------

Unfortunately, we can't immediately fix this, because doing so requires marking EndTxnRequest.Deadline as non-nullable. This causes mixed-version complications because v21.2 processes won't handle an empty deadline timestamp the same as they would a nil deadline timestamp. So to get to where we want to be, we first need to handle empty deadlines the same as nil deadlines server-side in the v22.1 release. Then we can address the rest of this in v22.2.

Jira issue: CRDB-12008
@nvanbenschoten nvanbenschoten added C-performance Perf of queries or internals. Solution not expected to change functional behavior. A-kv-transactions Relating to MVCC and the transactional model. labels Dec 22, 2021
@nvanbenschoten nvanbenschoten self-assigned this Dec 22, 2021
@blathers-crl blathers-crl bot added the T-kv KV Team label Dec 22, 2021
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Dec 22, 2021
@nvanbenschoten
kv: treat empty transaction deadlines identically to nil deadlines
c6dc23e 
This is as much as we can do for cockroachdb#74224 this release (v22.1). It sets us
up to do the rest in the following release (v22.2).
@nvanbenschoten nvanbenschoten mentioned this issue Dec 22, 2021
Merged
craig bot pushed a commit that referenced this issue Dec 23, 2021
@nvanbenschoten
Merge #74223 #74225
053fe7d 
74223: roachprod: configure logging in roachprod library r=tbg a=healthy-pod

This patch moves `pkg/cmd/roachtest/logger` to `pkg/roachprod/logger`
to be used as a logger by the roachprod library. `roachtest` and the
roachprod binary are then configured to use/pass instances of `logger`
as needed by `pkg/roachprod`.

All calls to `fmt.Printf` and `fmt.Println` were removed from
`pkg/roachprod` except in few cases specific to the roachprod
binary. In the future, using `fmt.Printf` and `fmt.Println` in
the library (i.e. `pkg/roachprod`) should be discouraged.

Closes #73234.

Release note: None

74225: kv: treat empty transaction deadlines identically to nil deadlines r=irfansharif a=nvanbenschoten

This is as much as we can do for #74224 this release (v22.1). It sets us up to do the rest in the following release (v22.2).

Co-authored-by: Ahmad Abedalqader <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
@ajwerner
Copy link
Contributor

This issue highlights how hopeless a transition to vtprotobuf and its pooling feels. If we had infinite elbow grease, is that the route we'd take?

gustasva pushed a commit to gustasva/cockroach that referenced this issue Jan 4, 2022
@nvanbenschoten
kv: treat empty transaction deadlines identically to nil deadlines
52b3aed 
This is as much as we can do for cockroachdb#74224 this release (v22.1). It sets us
up to do the rest in the following release (v22.2).
@ajwerner
Copy link
Contributor

@nvanbenschoten we can do this now, yeah?

@nvanbenschoten nvanbenschoten mentioned this issue Aug 1, 2022
Merged
craig bot pushed a commit that referenced this issue Aug 2, 2022
@nvanbenschoten
Merge #85408
b1de533 
85408: kv: don't heap allocate transaction deadline timestamp r=nvanbenschoten a=nvanbenschoten

Fixes #74224.
Second half of #74225.

This commit changes `EndTxnRequest.Deadline` from a nullable `*hlc.Timestamp` to
a non-nullable `hlc.Timestamp`. This avoids the need to heap allocate the
transaction deadline.

In #74224, we saw that in a run of sysbench's `oltp_point_select` workload, this
was the 9th most frequent source of heap allocations, accounting for over **1%**
of total allocations.

```
----------------------------------------------------------+-------------
      flat  flat%   sum%        cum   cum%   calls calls% + context
----------------------------------------------------------+-------------
...
----------------------------------------------------------+-------------
                                          63603658   100% |   github.com/cockroachdb/cockroach/pkg/sql/catalog/descs.(*leasedDescriptors).maybeUpdateDeadline /go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/leased_descriptors.go:217
  63603658  1.06% 13.90%   63603658  1.06%                | github.com/cockroachdb/cockroach/pkg/kv.(*Txn).UpdateDeadline /go/src/github.com/cockroachdb/cockroach/pkg/kv/txn.go:784
----------------------------------------------------------+-------------
```

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
@craig craig bot closed this as completed in d06a355 Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvanbenschoten nvanbenschoten

Labels
A-kv-transactions Relating to MVCC and the transactional model. C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-kv KV Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

kv: don't heap allocate transaction deadline timestamp nvanbenschoten/cockroach
2 participants
@ajwerner @nvanbenschoten