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

backport-2.1: storage,kv: remove some allocations #29077

Merged
merged 1 commit into from
Aug 27, 2018

Conversation

jordanlewis
Copy link
Member

Backport 1/1 commits from #29075.

/cc @cockroachdb/release


Guard an expensive log.V in a log.ExpensiveLogEnabled.

Pass roachpb.Lease by value in one situation in which allocating it is
pointless.

cc @benesch

This seems to improve read-only kv performance by almost 5%!

Release note: None

Guard an expensive log.V in a log.ExpensiveLogEnabled.

Pass roachpb.Lease by value in one situation in which allocating it is
pointless.

Release note: None
@jordanlewis jordanlewis requested review from benesch, nvanbenschoten and a team August 26, 2018 19:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jordanlewis
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 27, 2018

Build failed

@jordanlewis
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 27, 2018

Build failed

@jordanlewis
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 27, 2018

Build failed

@jordanlewis
Copy link
Member Author

This flaky test is also seemingly flaky on release 2.1. let's try once more

bors r+

@tbg
Copy link
Member

tbg commented Aug 27, 2018

@jordanlewis could you comment on why these fail before you retry? I know it's annoying and I wish we had tooling for it, but IMO it's still worth doing or flakiness will go up not down.

@jordanlewis
Copy link
Member Author

Yeah, sorry. The flake is a test timeout. The test gets wedged with some locks waiting on each other. I couldn't reproduce locally until I moved the test timeout to 10s in stressrace, but then I could also reproduce on release-2.1. Mainly confused how TC hits this so frequently but stressrace doesn't turn it up without very low timeouts.

@craig
Copy link
Contributor

craig bot commented Aug 27, 2018

Build failed

@jordanlewis
Copy link
Member Author

That was a different failure: TestReopenConnection. What the heck? I can't believe that this would be caused by this patch. And yet, I don't see this flaking elsewhere .

@tbg
Copy link
Member

tbg commented Aug 27, 2018

I've got something similar (different test though) going on over in #29033 (comment). Not sure what to make of it yet.

@jordanlewis
Copy link
Member Author

One more try. I still can't see the connection and I can't reproduce with stress race.

bors r+

@jordanlewis
Copy link
Member Author

I think the bors batch crashed? World's hardest PR to merge

bors r+

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm_strong: (maybe that will help)

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

craig bot pushed a commit that referenced this pull request Aug 27, 2018
29077: backport-2.1: storage,kv: remove some allocations r=jordanlewis a=jordanlewis

Backport 1/1 commits from #29075.

/cc @cockroachdb/release

---

Guard an expensive log.V in a log.ExpensiveLogEnabled.

Pass roachpb.Lease by value in one situation in which allocating it is
pointless.

cc @benesch

This seems to improve read-only kv performance by almost 5%!

Release note: None


29103: release-2.1: base: make TestValidateAddrs portable r=knz a=knz

Backport 1/1 commits from #29101.

/cc @cockroachdb/release

---

On (at least) osx the port name resolution error is different. This patch
modifies the test to catch that.

Release note: None


29121: release-2.1: opt: fix LookupJoinDef interning, add tests r=RaduBerinde a=RaduBerinde

Backport 1/1 commits from #29117.

/cc @cockroachdb/release

---

Fixing an omission I noticed in internLookupJoinDef and adding missing
tests for interning defs.

Release note: None


Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 27, 2018

Build succeeded

@craig craig bot merged commit 497feb7 into cockroachdb:release-2.1 Aug 27, 2018
@jordanlewis
Copy link
Member Author

image

@jordanlewis jordanlewis deleted the backport2.1-29075 branch August 27, 2018 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants