Conversation
commit c0de4a1 Author: Ewout ter Hoeven <[email protected]> Date: Sun Jan 21 14:43:08 2024 +0100 Add CI workflow for performance benchmarks (mesa#1983) This commit introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on `pull_request_target` events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.
This reverts commit 9485087.
|
How can I trigger the benchmark stuff? |
|
Performance benchmarks:
|
|
The benchmark init numbers make little sense to me. My changes only relate to |
|
Can you replicate them locally? Maybe with some more replications/seeds (modify |
I'll try. One possible solution, more generally, is to ensure the same seeds are used in both benchmarks. I'll look at the code and open a separate PR if needed. |
Should already be happening: But if you see room for improvement, please do! Also check: #1983 (comment) |
I ran it locally with the default settings. The results are more in line with what I would expect. It seems the init is particularly noisy. Not really surprising but good to know.
|
|
This is ready for review and merging. When merging, please squash and merge. |
Corvince
left a comment
Corvince
left a comment
There was a problem hiding this comment.
I am wondering if a similar logic could be applied to there other methods or in place of the current _update method. I would think similar performance improvements might be possible?
It's a bit tricky. Both |
|
This PR is starting to look ready to me. How good is the test coverage of this part of the code? |
EwoutH
added
trigger-benchmarks
|
Performance benchmarks:
|
|
Do we have any idea why this could slow down the shelling large init? It happened on both CI runs, with remarkable consistency. |
We added testcode as part of #2007. This already covers
Yes, but not on my machine. Moreover, the init code is unrelated to this change. Shuffle is not called in |
|
@quaquel protip: with https://github.com/projectmesa/mesa/blob/6a4ea54d6dade48591db512d3f7ada18ec4d42ff/CONTRIBUTING.rst?plain=1#L30, when you do |
|
Squashed and merged with the commit message |
| self.random.shuffle(weakrefs) | ||
|
|
||
| if inplace: | ||
| self._agents.data = {entry: None for entry in weakrefs} |
There was a problem hiding this comment.
How come in this line there is no need for (agent := ref()) is not None), but it is needed in L204?
There was a problem hiding this comment.
When you create a new AgentSet, it has its own internal WeakkeyDict. Each WeakkeyDict creates its own weakrefs.
As can be seen in the docs, weakref.ref takes an optional callback. In the case of WeakkeyDict, this callback is a method on the dict itself (i.e., self.remove). So, when not shuffling in place, I have to unpack the weakrefs to ensure the new WeakkeyDict behaves correctly.
Moreover, AgentSet takes an iteratable of agents as an argument. Not an iterable of weakref. ref instances.
Hope this clarifies.
4 participants
This speeds up shuffle by shuffling the actual weakrefs, rather than first turning them back into hard refs, shuffling them, and turning them back into weakness (which is what the old code did).
One probably controversial choice is that I have shifted to default inplace shuffle. Feel free to let me know what you think. I am fine either way but inplace is quite a bit faster (and the default behavior of random.shuffle).