Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uses serialized graph in tile model #9753

Merged
merged 7 commits into from
Sep 20, 2023

Conversation

aarongundel
Copy link
Contributor

Uses serialized graph in tile model. This was already used in quite a few places here, there were only a few changes to make.

arches/app/models/tile.py Outdated Show resolved Hide resolved
@aarongundel aarongundel requested a review from chiatt September 15, 2023 22:06
@jacobtylerwalls jacobtylerwalls self-requested a review September 18, 2023 19:19
@jacobtylerwalls jacobtylerwalls linked an issue Sep 18, 2023 that may be closed by this pull request
@@ -598,8 +614,14 @@ def get_blank_tile_from_nodegroup_id(nodegroup_id, resourceid=None, parenttile=N
tile.resourceinstance_id = resourceid
tile.parenttile = parenttile
tile.data = {}
tile.load_serialized_graph()
Copy link
Member

Choose a reason for hiding this comment

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

Because load_serialized_graph() does a db query, we're probably giving back all the intended performance savings. I noticed the two implementations perform roughly the same (this is with a nodegroup from AfS):

python3 manage.py shell
>>> from timeit import timeit
>>> from arches.app.models.tile import Tile
>>> timeit("Tile.get_blank_tile_from_nodegroup_id('0434ccce-1d9f-11eb-a29f-024e0d439fdb')", globals=globals(), number=5000)  # pr
3.2668450839992147
>>> timeit("Tile.get_blank_tile_from_nodegroup_id('0434ccce-1d9f-11eb-a29f-024e0d439fdb')", globals=globals(), number=5000)  # dev branch
3.308565291983541

I can take a closer look at this tomorrow, but what do you think about that point in general, in other words only trying to do this (potential) optimization only if we already have the published graph in hand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that it's likely the caller of this method will not just grab the empty tile and be finished, but that they'll set some nodes in the tile and then save it back to the database - at which point this becomes a bit more valuable because then the serialized graph doesn't have to be loaded on save. So this is a prefetch of the the serialized graph for that tile object. However, it is true that this is not strictly needed here, and it's probably better left for when it is actually needed - the optimization is a bit premature.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Looking good, just a few suggestions.

arches/app/models/tile.py Outdated Show resolved Hide resolved
Comment on lines 602 to 625
for node in models.Node.objects.filter(nodegroup=nodegroup_id):
for node in nodes:
if node.datatype != "semantic":
Copy link
Member

Choose a reason for hiding this comment

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

Assuming we only have a queryset here, which might be the case after the outcome of the prior comment, this could be rewritten with .exclude() so that the exclusion of semantic nodes is done in the db and returns a smaller result set. Since we're only using the nodeid you could also select only that with values/values_list.

arches/app/models/tile.py Show resolved Hide resolved
arches/app/models/tile.py Outdated Show resolved Hide resolved
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

@aarongundel aarongundel merged commit 22332fb into dev/7.4.x Sep 20, 2023
@aarongundel aarongundel deleted the 9734-cached-graph-perf-tiles branch September 20, 2023 21:18
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.

Tile proxy model should leverage published graph
3 participants