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

runtime: remove no-op pointer writes in treap rotations #28420

Closed
wants to merge 1 commit into from
Closed

runtime: remove no-op pointer writes in treap rotations #28420

wants to merge 1 commit into from

Conversation

zhiqiangxu
Copy link
Contributor

@zhiqiangxu zhiqiangxu commented Oct 26, 2018 •
edited by bradfitz

No description provided.

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Oct 26, 2018
@gopherbot
Copy link

This PR (HEAD: 4b81a79) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/144999 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Gerrit User 13550:

Patch Set 1: Code-Review-1

This CL needs to explain why the code is unnecessary. Perhaps I'm missing something obvious, but the code doesn't seem unnecessary to me by just reading the commit message and diff.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

jcs090218
jcs090218 reviewed Nov 10, 2018
View reviewed changes
Copy link

@jcs090218 jcs090218 left a comment

Choose a reason for hiding this comment

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

@zhiqiangxu I might need to explain why the code is unnecessary.

src/runtime/sema.go
@@ -373,19 +373,11 @@ Found:
func (root *semaRoot) rotateLeft(x *sudog) {
// p -> (x a (y b c))
p := x.parent
a, y := x.prev, x.next
Copy link

Choose a reason for hiding this comment

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

I checked the whole file but the code not seems to be unnecessary. Even though, I think the code remain here is to exemplify what was the logic here before they code this?

AnikHasibul
AnikHasibul approved these changes Nov 11, 2018
View reviewed changes
src/runtime/sema.go

y.prev = x
x.parent = y
y.next = c
Copy link

Choose a reason for hiding this comment

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

Why did they even use this before? What was the logic?

@gopherbot
Copy link

Message from Rick Hudson:

Patch Set 1: Code-Review+2

Patch Set 1: Code-Review-1

This CL needs to explain why the code is unnecessary. Perhaps I'm missing something obvious, but the code doesn't seem unnecessary to me by just reading the commit message and diff.

The point is that if you want to rotate left the parent x with the offspring y, ie change
(x a (y b c)) into (y (x a b) c) then x.prev (ie a) and y.next (ie c) remain unchanged.
(x a (_ _ )) into ( (x a ) ) and ( _ (y _ c)) into (y ( _ _) c).
The original code includes redundant assignments of x.prev to a and y.next to c. Similar arguments can be made for rotate right.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Daniel Martí:

Patch Set 1:

The point is that if you want to rotate left the parent x with the offspring y, ie change
(x a (y b c)) into (y (x a b) c) then x.prev (ie a) and y.next (ie c) remain unchanged.
(x a (_ _ )) into ( (x a ) ) and ( _ (y _ c)) into (y ( _ _) c).
The original code includes redundant assignments of x.prev to a and y.next to c. Similar arguments can be made for rotate right.

Fair enough. I'd still include this clarification in the commit message, though :)

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Josh Bleecher Snyder:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Rick Hudson:

Patch Set 1: Run-TryBot+1

Trybots

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ee47dcf1

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 1:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/ee47dcf1/freebsd-amd64-12_0_13e47607.log

Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 1: TryBot-Result-1

1 of 19 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/ee47dcf1/freebsd-amd64-12_0_13e47607.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Rick Hudson:

Patch Set 1:

Trybot fail seems unrelated, try again.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Rick Hudson:

Patch Set 1:

Patch Set 1:

Trybot fail seems unrelated, try again.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Russ Cox:

Patch Set 1: Code-Review-2

Please change the first line of the commit message to the standard form, with a prefix giving the package name and a clearer summary of the change:

runtime: remove no-op pointer writes in treap rotations

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Russ Cox:

Patch Set 1:

Also please update your Author line to use a full name. Thanks.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot gopherbot force-pushed the master branch 3 times, most recently from 53bd915 to 6139019 Compare October 3, 2019 21:09
@bradfitz bradfitz changed the title rm useless code runtime: remove no-op pointer writes in treap rotations Oct 10, 2019
@gopherbot
Copy link

Message from Brad Fitzpatrick:

Patch Set 3: Patch Set 2 was rebased

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Brad Fitzpatrick:

Patch Set 3: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 3:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=c73389c0

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 3: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/144999.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

This PR is being closed because golang.org/cl/144999 has been merged.

@gopherbot gopherbot closed this Oct 10, 2019
gopherbot pushed a commit that referenced this pull request Oct 10, 2019
@zhiqiangxu @bradfitz
runtime: remove no-op pointer writes in treap rotations
26ff21d 
Change-Id: If5a272f331fe9da09467efedd0231a4ce34db0f8
GitHub-Last-Rev: 4b81a79
GitHub-Pull-Request: #28420
Reviewed-on: https://go-review.googlesource.com/c/go/+/144999
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcs090218 jcs090218 jcs090218 left review comments

@AnikHasibul AnikHasibul AnikHasibul approved these changes

Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zhiqiangxu @gopherbot @jcs090218 @AnikHasibul @googlebot