Summary

Speed up looking up actions by attachment point.

Review Request #14639 — Created Oct. 16, 2025 and updated Nov. 10, 2025, 8:53 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Now, when registering an action, the action gets placed in a cache of
attachment points to actions. Unregistering will remove that action from
the cache. This makes lookup very fast.

Testing Done

Unit tests pass.

Tested various parts of the UI, making sure the right actions are in
the right places.

Commits

Summary	ID
Speed up looking up actions by attachment point. When populating the UI, we fetch actions based on various attachment points. When we do this, the registry has to iterate through every action every time, returning each one matching the attachment point. This is algorithmically slow, and only gets worse as we expand usage of actions. Now, when registering an action, the action gets placed in a cache of attachment points to actions. Unregistering will remove that action from the cache. This makes lookup very fast.	1aadbfc839c86cccf3f52ceb3d31c692005070a4

Summary

Speed up looking up actions by attachment point.

When populating the UI, we fetch actions based on various attachment points. When we do this, the registry has to iterate through every action every time, returning each one matching the attachment point. This is algorithmically slow, and only gets worse as we expand usage of actions. Now, when registering an action, the action gets placed in a cache of attachment points to actions. Unregistering will remove that action from the cache. This makes lookup very fast.

1aadbfc839c86cccf3f52ceb3d31c692005070a4

Issues

Description	From	Last Updated
We should use a walrus operator here to make a variable out of action.parent_id, like you did below.	maubin	Nov. 6, 2025, 2:55 a.m.
Can use action_id here too.	maubin	Nov. 6, 2025, 3:01 a.m.
Might as well do the walrus operator here too.	maubin	Nov. 6, 2025, 2:59 a.m.
I'm assuming this is part of the future proofing, where in the future action.attachment can possibly be a list of …	maubin	Nov. 6, 2025, 2:59 a.m.
Same notes here about the attachment variable.	maubin	Nov. 6, 2025, 2:59 a.m.
We need to add from __future__ import annotations to this file, especially because you're using dict[...] syntax for type hints.	david	Nov. 8, 2025, 10:23 p.m.
Perhaps we should make _by_attachment_point a defaultdict(dict)?	david	Nov. 8, 2025, 10:26 p.m.
Just in case an action failed to register properly, we probably should catch KeyError here and log.	david	Nov. 8, 2025, 10:29 p.m.
Do we want to make it so we clean up empty dicts from the cache after actions are unregistered?	david	Nov. 8, 2025, 11:07 p.m.
These older tests are still using the global actions_registry. Can we update these to construct a _TestActionsRegistry and then get …	david	Nov. 9, 2025, 12:01 a.m.

flake8 passed.

JSHint passed.

reviewboard/actions/registry.py (Diff revision 1)

The issue has been dropped. Show all issues

We should use a walrus operator here to make a variable out of action.parent_id, like you did below.

chipx86 Nov. 6, 2025, 2:58 a.m.

I'm going to drop this one because it's code that's being fully rewritten in a later change. This affects pretty much everything in registry.py that's relevant here. I want to keep the file largely untouched up until that point for code archaeology reasons.

reviewboard/actions/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can use action_id here too.
```
reviewboard/actions/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Might as well do the walrus operator here too.
```

reviewboard/actions/registry.py (Diff revision 1)

The issue has been resolved. Show all issues

I'm assuming this is part of the future proofing, where in the future action.attachment can possibly be a list of strings?

We should name the attachments list attachments instead of having all variables be attachment. Note that the for loop variable is also named attachment. After the loop runs, the attachment list variable would now point to the last item in the list instead of the list itself. Doesn't really matter since we don't use the variable again, but its confusing and will cause problems in case we do use it in the future.

How about something like this

attachments = action.attachment

if attachments:
    if isinstance(attachments, str):
        attachments = [attachments]

    for attachment in attachments:
        ...

chipx86 Nov. 6, 2025, 2:58 a.m.

I probably should just nuke this, because it's nuked in a future change as part of the big change I mentioned in my earlier comment. All this is going away. I can just undo it for now.

reviewboard/actions/registry.py (Diff revision 1)

The issue has been resolved. Show all issues

Same notes here about the attachment variable.

Change Summary:


Removed the future-proofing for lists of attachment IDs, since that method won't be used.
Used some walrus operators in new code.

Description:

		When populating the UI, we fetch actions based on various attachment
		points. When we do this, the registry has to iterate through every
		action every time, returning each one matching the attachment point.
		This is algorithmically slow, and only gets worse as we expand usage of
		actions.

		Now, when registering an action, the action gets placed in a cache of
		attachment points to actions. Unregistering will remove that action from
		the cache. This makes lookup very fast.
-
-		There's also a bit of future-proofing thrown in for upcoming support for
-		actions that attach to multiple attachment points (part of a larger set
-		of work for enhanced actions in Review Board 7.1).

Commits:

	Summary	ID
	Speed up looking up actions by attachment point. When populating the UI, we fetch actions based on various attachment points. When we do this, the registry has to iterate through every action every time, returning each one matching the attachment point. This is algorithmically slow, and only gets worse as we expand usage of actions. Now, when registering an action, the action gets placed in a cache of attachment points to actions. Unregistering will remove that action from the cache. This makes lookup very fast. There's also a bit of future-proofing thrown in for upcoming support for actions that attach to multiple attachment points (part of a larger set of work for enhanced actions in Review Board 7.1).	145b2020d753a75f8bc15d98b170e4703441afb9
	Speed up looking up actions by attachment point. When populating the UI, we fetch actions based on various attachment points. When we do this, the registry has to iterate through every action every time, returning each one matching the attachment point. This is algorithmically slow, and only gets worse as we expand usage of actions. Now, when registering an action, the action gets placed in a cache of attachment points to actions. Unregistering will remove that action from the cache. This makes lookup very fast.	cf043b93c85f24b3bb432ce50774d56f1540c89a

Diff:

Revision 2 (+270 -38)

Show changes

	reviewboard/actions/registry.py
	reviewboard/actions/tests/test_registry.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/actions/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

We need to add from __future__ import annotations to this file, especially because you're using dict[...] syntax for type hints.

reviewboard/actions/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Perhaps we should make _by_attachment_point a defaultdict(dict)?

reviewboard/actions/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Just in case an action failed to register properly, we probably should catch KeyError here and log.

reviewboard/actions/tests/test_registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Do we want to make it so we clean up empty dicts from the cache after actions are unregistered?

chipx86 Nov. 9, 2025, 12:01 a.m.

Sure. Probably a rare enough event, but cleanup is easy.

reviewboard/actions/tests/test_registry.py (Diff revision 2)

The issue has been dropped. Show all issues

These older tests are still using the global actions_registry. Can we update these to construct a _TestActionsRegistry and then get rid of the tearDown() implementation instead?

chipx86 Nov. 9, 2025, 12:01 a.m.

I'll tackle this separately. I don't want to add any more to this change.

Change Summary:


Moved to a defaultdict for the attachment point lookup.
Added logging when an action ID is missing from an attachment point during unregistration.
Empty attachment points are removed from the lookup table after unregistration.

Commits:

	Summary	ID
	Speed up looking up actions by attachment point. When populating the UI, we fetch actions based on various attachment points. When we do this, the registry has to iterate through every action every time, returning each one matching the attachment point. This is algorithmically slow, and only gets worse as we expand usage of actions. Now, when registering an action, the action gets placed in a cache of attachment points to actions. Unregistering will remove that action from the cache. This makes lookup very fast.	cf043b93c85f24b3bb432ce50774d56f1540c89a
	Speed up looking up actions by attachment point. When populating the UI, we fetch actions based on various attachment points. When we do this, the registry has to iterate through every action every time, returning each one matching the attachment point. This is algorithmically slow, and only gets worse as we expand usage of actions. Now, when registering an action, the action gets placed in a cache of attachment points to actions. Unregistering will remove that action from the cache. This makes lookup very fast.	1aadbfc839c86cccf3f52ceb3d31c692005070a4

Diff:

Revision 3 (+280 -38)

Show changes

	reviewboard/actions/registry.py
	reviewboard/actions/tests/test_registry.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```