build: compile with golang 1.16 #3803

tsachiherman · 2022-03-19T14:34:05Z

Summary

This PR upgrade the go-algorand repository to use go 1.16 while adding workarounds for golang/go#44343.

Test Plan

Use performance testing to verify no regressions.

codecov-commenter · 2022-03-19T21:50:11Z

Codecov Report

Merging #3803 (6399921) into master (35be745) will increase coverage by 0.12%.
The diff coverage is 30.00%.

@@            Coverage Diff             @@
##           master    #3803      +/-   ##
==========================================
+ Coverage   49.88%   50.01%   +0.12%     
==========================================
  Files         392      392              
  Lines       68722    68504     -218     
==========================================
- Hits        34284    34259      -25     
+ Misses      30682    30491     -191     
+ Partials     3756     3754       -2

Impacted Files	Coverage Δ
network/dialer.go	`64.00% <0.00%> (ø)`
network/rateLimitingTransport.go	`48.14% <0.00%> (-1.86%)`	⬇️
network/wsNetwork.go	`62.99% <100.00%> (ø)`
util/condvar/timedwait.go	`100.00% <100.00%> (ø)`
agreement/cryptoVerifier.go	`70.21% <0.00%> (-4.97%)`	⬇️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
node/node.go	`23.26% <0.00%> (-1.85%)`	⬇️
cmd/tealdbg/debugger.go	`71.42% <0.00%> (-0.99%)`	⬇️
data/abi/abi_type.go	`87.67% <0.00%> (-0.95%)`	⬇️
ledger/acctupdates.go	`68.51% <0.00%> (-0.93%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35be745...6399921. Read the comment docs.

algojack

lgtm

tolikzinovyev

We should also consider upgrading (in a separate PR) to go 1.17, since 1.18 is out and 1.16 is therefore deprecated.

tolikzinovyev · 2022-03-21T16:09:38Z

shared/pingpong/pingpong.go

@@ -882,7 +883,7 @@ func (pps *WorkerState) sendFromTo(
 			timeCredit -= took
 			if timeCredit > 0 {
 				time.Sleep(timeCredit)


Why not use NanoSleep() here?

No need to be super-accurate here, since the timeCredit mechanism would compensate by dropping sleeps on subsequent iterations.

tolikzinovyev · 2022-03-21T16:10:40Z

shared/pingpong/pingpong.go

@@ -882,7 +883,7 @@ func (pps *WorkerState) sendFromTo(
 			timeCredit -= took
 			if timeCredit > 0 {
 				time.Sleep(timeCredit)
-				timeCredit = time.Duration(0)
+				timeCredit -= time.Since(now)


This seems better, but still not as precise as #3478.

#3478 was removing the credits logic, which would make it worse if we were to generate > 1000 tps.

why exactly?

In #3478, you were calculating the next timestamp based on the current one by adding the "1/tps". This is generally correct. However, if we want to have 1000 tps, and half of the iterations takes 0.5ms while the other half takes 1.5ms, the effective TPS would be 1.5 x 500 + 1 x 500 = 1250 sec / 1000 txn -> 800 tps. That's why we're using the credit algorithm.

This is not what #3478 does. I think the PR description in #3478 explains how it works.

The description in #3478 does match the implementation.. But the goal was not to fix it for the 115 tps use case. The goal was to fix it so it would work correctly with the go 1.16 bug. If you'll take your implementation and try to use it for more than 1K tps from a single pingpong on go 1.16 it wouldn't work. With this implementation, it would.
( we need to have that in order to be able to test 10K TPS ).

Why will my implementation not work?

let's say we set the tps to 2000. In that case, each sleep should take 0.5ms. But because of this bug, it would take 1ms. That would limit the tps to 1000 ( assuming that the iteration is free ).

Btw - I do like the simplicity in your implementation, but I think we should work a bit more on it. Let's rebase your branch based on this one, and conduct some further testing against high-volume TPS ? I want to see that it would work correctly both in the cases of random iteration timing as well as high TPS.

let's say we set the tps to 2000. In that case, each sleep should take 0.5ms. But because of this bug, it would take 1ms. That would limit the tps to 1000 ( assuming that the iteration is free ).

No, because some sleeps will be skipped. But I'm definitely not against using nanosleep for more precision.

Btw - I do like the simplicity in your implementation, but I think we should work a bit more on it. Let's rebase your branch based on this one, and conduct some further testing against high-volume TPS ? I want to see that it would work correctly both in the cases of random iteration timing as well as high TPS.

Sure!

tolikzinovyev · 2022-03-21T16:15:58Z

test/e2e-go/features/participation/participationRewards_test.go

@@ -358,7 +358,7 @@ func TestRewardRateRecalculation(t *testing.T) {
 	r.NoError(fixture.WaitForRoundWithTimeout(rewardRecalcRound - 1))
 	balanceOfRewardsPool, roundQueried := fixture.GetBalanceAndRound(rewardsAccount)
 	if roundQueried != rewardRecalcRound-1 {
-		r.FailNow("got rewards pool balance on round %d but wanted the balance on round %d, failing out", rewardRecalcRound-1, roundQueried)
+		r.FailNow("", "got rewards pool balance on round %d but wanted the balance on round %d, failing out", rewardRecalcRound-1, roundQueried)


what does this do?

The FailNow parameters were just wrong, and instead of failing with the desired error message, it would fail during FailNow to format the parameters.

tolikzinovyev · 2022-03-21T16:29:21Z

util/condvar/timedwait.go

@@ -42,7 +44,7 @@ func TimedWait(c *sync.Cond, timeout time.Duration) {
 			// thread hasn't gotten around to calling c.Wait()
 			// yet, so the c.Broadcast() did not wake it up.
 			// Sleep for a second and check again.
-			<-time.After(time.Second)
+			time.Sleep(time.Second)


Why not use util.NanoSleep()?

I used NanoSleep only in places that have time-criticality. On this one, we clearly don't care of it would take exactly 1 second or 1 second and 1 ms.

tolikzinovyev · 2022-03-21T16:30:37Z

util/sleep_linux.go

+//
+// You should have received a copy of the GNU Affero General Public License
+// along with go-algorand.  If not, see <https://www.gnu.org/licenses/>.
+


I think this needs // +build linux.

no need. it's implied by the "_linux" postfix to the filename.

util/sleep_linux.go

tsachiherman · 2022-03-21T18:14:01Z

We should also consider upgrading (in a separate PR) to go 1.17, since 1.18 is out and 1.16 is therefore deprecated.

agree, but as you've suggested - a separate PR.

tsachiherman added 3 commits March 18, 2022 09:06

try to fix pingpong.

0877b99

update

4bf0a54

update

af0d45c

tsachiherman added Enhancement Team Carbon-11 labels Mar 19, 2022

tsachiherman self-assigned this Mar 19, 2022

tsachiherman added 9 commits March 19, 2022 10:47

update

47408b8

lint

de54071

update

f9a369f

few more fixes

8a0a7e8

update

4b41e70

update pingpong

9a85b93

fix nanosleep parameters.

d06a3d7

fix test.

97f5718

accts

d2487a7

tsachiherman added 2 commits March 19, 2022 18:06

use Duration instead of int64

30edc32

bugfix.

2ccb38c

tsachiherman requested review from algojack and tolikzinovyev March 20, 2022 15:13

algojack previously approved these changes Mar 21, 2022

View reviewed changes

tolikzinovyev reviewed Mar 21, 2022

View reviewed changes

break up long line

9ccfba4

tsachiherman dismissed algojack’s stale review via 9ccfba4 March 21, 2022 18:10

tsachiherman added 2 commits March 21, 2022 14:15

style

fcf5f0f

better comment

6399921

tolikzinovyev approved these changes Mar 21, 2022

View reviewed changes

tsachiherman merged commit bc19540 into algorand:master Mar 21, 2022

tsachiherman deleted the tsachi/go116upgrade branch March 21, 2022 21:34

cce mentioned this pull request Mar 25, 2022

build: use 1.16.15 instead of 1.16.11 #3833

Merged

egieseke mentioned this pull request Apr 22, 2022

go-algorand 3.6.0-beta Release PR #3909

Closed

Algo-devops-service mentioned this pull request Apr 26, 2022

go-algorand 0.0.0-beta Release PR #3922

Closed

This was referenced Apr 27, 2022

go-algorand 3.6.0-beta Release PR #3923

Merged

go-algorand 3.6.0-beta Release PR #3934

Merged

This was referenced May 2, 2022

go-algorand 3.6.1-beta Release PR #3943

Merged

go-algorand 3.6.2-beta Release PR #3947

Merged

Algo-devops-service mentioned this pull request May 7, 2022

go-algorand 3.6.2-stable Release PR #3960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: compile with golang 1.16 #3803

build: compile with golang 1.16 #3803

tsachiherman commented Mar 19, 2022 •

edited

Loading

codecov-commenter commented Mar 19, 2022 •

edited

Loading

algojack left a comment

tolikzinovyev left a comment

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022 •

edited

Loading

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tsachiherman Mar 21, 2022

tolikzinovyev Mar 21, 2022

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tolikzinovyev Mar 21, 2022

tsachiherman Mar 21, 2022

tsachiherman commented Mar 21, 2022

build: compile with golang 1.16 #3803

build: compile with golang 1.16 #3803

Conversation

tsachiherman commented Mar 19, 2022 • edited Loading

Summary

Test Plan

codecov-commenter commented Mar 19, 2022 • edited Loading

Codecov Report

algojack left a comment

Choose a reason for hiding this comment

tolikzinovyev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman commented Mar 21, 2022

tsachiherman commented Mar 19, 2022 •

edited

Loading

codecov-commenter commented Mar 19, 2022 •

edited

Loading

tsachiherman Mar 21, 2022 •

edited

Loading