Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

pytest.benchmark #19

Merged
merged 11 commits into from
Jul 7, 2018
Merged

pytest.benchmark #19

merged 11 commits into from
Jul 7, 2018

Conversation

RSabet
Copy link
Collaborator

@RSabet RSabet commented Jul 4, 2018

do you want something like this?

@mre
Copy link
Owner

mre commented Jul 5, 2018

Yeah exactly!
Seems like we just need to port the remaining tests, right? If that is true I would merge it and we can port the tests one by one.
Can you reintegrate the master? Seems like the Travis build fails for the exact same reason as yesterday and I've merged @wdv4758h's fix in #17 now. 😅

@RSabet
Copy link
Collaborator Author

RSabet commented Jul 5, 2018

had to rename benchmark to benchmarks, hope this doesn't break anything and rename benchmark.py to test_benchmark.py

Tests are working fine: pytest tests

benchmarks can be invoked:

  • just hyperjson: pytest benchmarks
  • compare to other libraries: pytest benchmarks --compare-other-engines

for some reason '100 arrays dict' - will open an issue since it is too late

@mre
Copy link
Owner

mre commented Jul 6, 2018

Great progress. 😊
Tested on my machine and it works beautifully. Just a few nits before we can merge this:
Could you also adjust the Makefile to use pytest commands instead of my previous, hacky approach?
Something like this:

.PHONY: test
test:
	pytest tests

.PHONY: bench
bench:
	pytest benchmarks

.PHONY: bench-all
bench-all:
	pytest benchmarks --compare-other-engines	

'int': 100100100,
'float': 100999.123456
}
for x in range(256):
Copy link
Owner

@mre mre Jul 6, 2018

Choose a reason for hiding this comment

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

I'd prefer to have a separate block per test object.

doubles = [sys.maxsize * random.random() for x in range(256)]

unicode_strings = [ "نظام  لولاية ا...رعيا لابوين عمانيين " for x in range(256)]

and so on.
The advantage would be that it's easier to reason about what the final test object might look like and adding/removing a test object just means adding/removing a block.

The disadvantage is, that there's some redundancy, but I think it's worth it.
Also, we could also extract that magic number 256 to some variable.

@wdv4758h
Copy link
Contributor

wdv4758h commented Jul 6, 2018

Here comes a result on one of my machine. hyperjson is at 3rd or 4th in many benchmarks, but it's at the 1st for:

  • test_loads[hyperjson-256 doubles array]
  • test_dumps[hyperjson-256 doubles array]
  • test_loads[hyperjson-256 unicode array]
  • test_dumps[hyperjson-256 unicode array]

For those benchmarks we are winning, we usually have clear margin. If this number is right, I think it's impressive for a super young project 😃

benchmark1
benchmark2
benchmark3
benchmark4

@wdv4758h
Copy link
Contributor

wdv4758h commented Jul 6, 2018

I think it will be good to have standard library's json module in the benchmark too, since we won't want to be slower than the default one. And 3rd implementation are actually easy to be slower for composite objects 😛

@wdv4758h
Copy link
Contributor

wdv4758h commented Jul 6, 2018

In my test, we are slower than standard library's json in these benchmarks:

  • test_loads[hyperjson-100 dicts array]
  • test_loads[hyperjson-256 Trues array]
  • test_dumps[hyperjson-256 Trues array]
  • test_loads[hyperjson-256 ascii array]
  • test_loads[hyperjson-complex object]
  • test_dumps[hyperjson-complex object]
  • test_loads[hyperjson-composite object]
  • test_dumps[hyperjson-composite object]

@RSabet
Copy link
Collaborator Author

RSabet commented Jul 7, 2018

although tests and benchmarks work locally, i can't make them pass in travis-ci, can someone help?

@mre
Copy link
Owner

mre commented Jul 7, 2018

Hum... it looks like you ran into a pytest bug. 😅
If I move the benchmarks/conftest.py to the project root folder, it works for me.
Here's the issue: pytest-dev/pytest#1596
Have nothing against keeping the conftest.py in the project root for now. 😉

@RSabet RSabet merged commit b00264f into mre:master Jul 7, 2018
@mre
Copy link
Owner

mre commented Jul 7, 2018

Thanks for this @RSabet 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants