Skip to content

Conversation

@elhimov
Copy link

@elhimov elhimov commented Oct 17, 2025

I didn't forget about (remove if it is not applicable):

Related issues:

Closes TNTP-3733

@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch from e796751 to 4dd55cd Compare October 17, 2025 09:53
@coveralls
Copy link

coveralls commented Oct 17, 2025

Coverage Status

coverage: 91.616%. remained the same
when pulling 368521d on elhimov/tntp-3733-import-and-actualize-benchmarks
into d70dadd on master.

@elhimov elhimov requested review from AlexandrLitkevich, bigbes and oleg-jukovec and removed request for oleg-jukovec October 17, 2025 09:59
@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch 6 times, most recently from ece055f to fdd30f4 Compare October 20, 2025 12:05
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Please, fix the red CI.

.PHONY: bench
bench:
@echo "Running benchmarks"
@go test ./... -count=1 -bench=. -benchmem
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need the benchmem? If so - ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

-benchmem will show allocation count, I think it's necessary information for us to use.

Copy link
Author

Choose a reason for hiding this comment

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

We are interested in number of allocation and this option displays allocation statistic.

dec := msgpack.GetDecoder()
dec.Reset(&buf)

b.Run("Optional1Bench", func(b *testing.B) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it a go-idiomic practice to name the tests in the manner? Why do we need the CamelCase here? Why not just "optional simple bench" or so on?

Up to @bigbes .

Copy link
Collaborator

@bigbes bigbes Oct 21, 2025

Choose a reason for hiding this comment

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

Well, I don't have any strong opinion about naming of subtests. On one hand I'm preferring to have name that I can copy to cli to run subtest, but go-idiomatic way is to write clear names with spaces in lowercase.

Let's stick with go-idiomatic way there.

@bigbes
Copy link
Collaborator

bigbes commented Oct 21, 2025

Pls, add README.bench.md with:

  • instructions to run bench
  • your results of benches and you cpu/mem specs

@oleg-jukovec
Copy link
Collaborator

Pls, add README.bench.md with:

Probably we could just update the README.md instead of a separate document.

The README.md now is small and shouldn't become too big.

@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch 14 times, most recently from e8df32c to 2b4ac2e Compare October 24, 2025 10:09
@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch 2 times, most recently from 2bc1aaa to 952c6c3 Compare October 24, 2025 10:11
Closes TNTP-3733
@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch from 952c6c3 to 368521d Compare October 24, 2025 10:12
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.

4 participants