Hello everyone, I am Xiaolou.

Remember last time I found a Bug where Benchmark execution of Go would time out? Here’s the article, “I think I found a Go Bug?” .

Later, I submitted a PR to Go for a fix. I wanted to wait for the code to be merged and later I could boast that I was a Go Contributor, but things didn’t Go well. Today I will share this failed code submission.

First submission

As soon as I became aware of the Bug, I couldn’t wait to fix it, which led to this commit.

Before talking about the Code, let’s talk about the Go warehouse. Go is not directly hosted on Github, but a self-built Gerrit Code Review. Github is just a mirror warehouse, and all the issues and codes submitted on Github will be transported to Gerrit by a robot.

In addition, Go requires that the submission code must be associated with an issue, so I proposed one and answered yes by myself.

I described the problem I encountered, but the next day, I was considered as a repeat problem by a bighead and closed the issue

But I clicked in to take a closer look, and I should have nothing to do with what I said, they are discussing the single test timeout does not take effect, so I quibble about it.

It worked. The other big guy agreed with me, so I gave him a thumbs-up, but he also pointed out that my code had a problem.

Let’s move on to today’s topic. For the sake of explanation, I’ll pick out the problematic code segment first:

func (b *B) launch(a){...// n (int64) may overflow
   n = goalns * prevIters / prevns
   ...
}
Copy the code

Now that we know that n is going to overflow, isn’t that easy? Just add a judgment.

Overflow is not fully considered

The big guy says that my code is not secure enough to prevent int64 overflow, isn’t that the judgment of overflow?

Fortunately, the big man gave me a little guidance

I also sent you a demo code

Sure enough, “show me the code” is the best, simple point is that the overflow of a positive number into a negative number, and then overflow is a positive number, as long as enough overflow, the result can be positive or negative.

Have a test

The big guy also pointed out another problem, brother, your code should be tested!

I don’t commit much code to open source projects, but I know this, so why not this time? Mainly I feel that the single test is not very good to write, since the big guy put forward, brave scalp also have to write.

Second submission

In the second submission, the method used to determine int64 overflow was changed, and the reverse operation was used to restore it and compare it with the original value to see whether it overflowed. This method was used last time in C language courses in university

A unit test is also attached

The unit test explains a little bit:

The single test time of 150s is set. For each trial single test, the number of times is increased by 1. If the trial number exceeds 6 times, it indicates that there is a problem and the single test is terminated.

This code is executed before the above overflow judgment is added, it must fail, after the overflow judgment is added, it can be executed normally.

Then I waited for the reply for a long time. The development cycle of Go was six months. I almost forgot about it until an email reminded me one day.

High energy ahead, let’s see how another big guy reviews my code.

Commit Message is not canonical

First of all, the commit message is not standard. My commit message looks like this. I submitted it on Github and it was carried by a robot.

The advice given is

The Go commit Message has a document that describes it

The first line of the Commit message should be a short summary and indicate which packages are affected, followed by a blank line.

The main content of the commit message should detail the context of the change and explain its purpose. It should be complete, punctuated correctly, and do not use markup languages such as HTML or Markdown. Relevant information, such as benchmark data, also needs to be included.

If fixing a problem, use Fixes #12345 to associate issue #12345. If fixing only part of the problem, use Updates #12345. If fixing the golang.org/x/ library, Use Fixes golang/go#159.

A good example is as follows:

math: improve Sin, Cos and Tan precision for very large arguments

The existing implementation has poor numerical properties for large arguments, so use the McGillicutty algorithm to improve accuracy above 1e10.

The algorithm is described at wikipedia.org/wiki/McGill… Fixes #159

I need to change the commit message.

Consider integration testing

The single test raises a lot of questions, starting with this one

I changed the name of Benchmark’s single test package to be able to call unexported methods in the package, which wasn’t a good idea, but I didn’t think of another solution.

The next is that you should not directly call the unexposed cleanups and some internal variables, echoing the above.

You can use flag.Lookup to set flags.

Or consider using substitute for unit testing, integration testing Go integration testing in CMD/Go/testdata/script, this didn’t contact before, so don’t know, how to use the integration test specific can see CMD/Go/testdata/script/README

This shows that I am really a Go newbie and need to read and learn more. Testing is not only single test, Go also supports integrated testing.

The lack of annotation

Then see again

Here simulated 150S single test, big guy asked, this single test will really run 150s? If so, it’s too long!

If not, you didn’t explain it to me clearly

And this

How do you know the number of executions must be less than 6? Go doesn’t guarantee that.

The core problem with both of these questions is that there are no comments, and people don’t know what you’re thinking. If open source code is riddled with this kind of unreadable stuff, that’s not a problem.

First of all, for the first one, simulated 150s, it will not really run that long, because there is a limit on the number of trial times. If it exceeds 6 times, it will stop. How is this 6 times obtained? The answer is in I think I found a Go Bug.

The maximum number of times Benchmark runs on a method is 1e9 times, that is, 1,000,000,000 times. If the execution time of the method to be tested is very short, and when Benchmark takes a long time, the calculation will overflow, so the number of test executions will be this increasing sequence:

100, 10000, 1000000, 100000000, 100000001, 100000002……

In fact, it may be >4, maybe there is something wrong with my previous test, EMM…

Overflows need to be reconsidered

Will it be more reasonable to determine if goalns is greater than or equal to the maximum int64 * prevIters?

N = goalns * prevIters/prevns, Goalns is the set execution time in nanoseconds

Looks like I’m a little short. Hold your horses, and

How do you know if 100 * last also overflowed? So would it make more sense to use Float64 all the way?

Float64 is a very large float64 with a wide range.

One last word

Although I failed this time, I still gained something. After I finish my work, I will take time out to change, and maybe Merge will happen. Wish me good luck. By the way, the issue reference in the article

  • Github.com/golang/go/p…

Search attention wechat public number “bug catching master”, back-end technology sharing, architecture design, performance optimization, source code reading, problem solving, practice.