Skip to content

Add memoized variants of mappers - #90

Merged
inducer merged 4 commits into
inducer:mainfrom
kaushikcfd:memoized_mappers
May 3, 2022
Merged

Add memoized variants of mappers#90
inducer merged 4 commits into
inducer:mainfrom
kaushikcfd:memoized_mappers

Conversation

@kaushikcfd

@kaushikcfdkaushikcfd commented Apr 25, 2022

Copy link
Copy Markdown
Collaborator

Closes#89.

@kaushikcfdkaushikcfd mentioned this pull request Apr 25, 2022
Merged
3 tasks
@kaushikcfd
kaushikcfd marked this pull request as draft April 25, 2022 15:18
@kaushikcfd
kaushikcfdforce-pushed the memoized_mappers branch 7 times, most recently from b0db09e to 1a624beCompareApril 26, 2022 00:52
@kaushikcfd
kaushikcfd marked this pull request as ready for review April 26, 2022 02:01
@kaushikcfd
kaushikcfdforce-pushed the memoized_mappers branch 2 times, most recently from 26c5f17 to a697d47CompareApril 26, 2022 22:36
@matthiasdienermatthiasdiener mentioned this pull request Apr 27, 2022
Draft
@inducerinducer mentioned this pull request Apr 27, 2022
Open
@inducer
inducerforce-pushed the memoized_mappers branch from 1d0767f to 20633e3CompareMay 2, 2022 03:52
inducer
inducer reviewed May 2, 2022
View reviewed changes

@inducerinducer left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks! Sorry this sat so long, it fell through the cracks somehow. A few things below, hopefully straightforward to deal with.

Comment threadpymbolic/mapper/__init__.py Outdated
Comment threadpymbolic/mapper/__init__.py
Comment threadpymbolic/mapper/__init__.py Outdated
A cache key implementation assuming *expr* is immutable while elements
of *args*, *kwargs* aren't.
"""
return ((id(expr), expr), args, tuple(sorted(kwargs.items())))

@inducerinducerMay 2, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Use immutables for hashing? (but see below)

@kaushikcfdkaushikcfdMay 3, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Don't think there's any difference: https://gist.github.com/kaushikcfd/3b9656f5ce0f813044966309ea472edc

@inducerinducerMay 3, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for trying!

Comment threadpymbolic/mapper/__init__.py
Comment threadpymbolic/mapper/__init__.py
inducer
inducer reviewed May 3, 2022
View reviewed changes
Comment threadpymbolic/mapper/__init__.py
inducer
inducer reviewed May 3, 2022
View reviewed changes
Comment threadpymbolic/mapper/__init__.py
@kaushikcfd
kaushikcfdforce-pushed the memoized_mappers branch 3 times, most recently from f1373c7 to ad51ea5CompareMay 3, 2022 20:26
@kaushikcfd
kaushikcfd requested a review from inducerMay 3, 2022 20:55
@inducer
inducerforce-pushed the memoized_mappers branch from c61ff56 to dc96d2aCompareMay 3, 2022 22:16
@inducer
inducer enabled auto-merge (rebase) May 3, 2022 22:17
@inducer

inducer commented May 3, 2022

Copy link
Copy Markdown
Owner

LGTM! In it goes. (Some doc tweaks in dc96d2a.)

@inducer
inducer merged commit d343cf1 into inducer:mainMay 3, 2022
matthiasdiener
matthiasdiener reviewed May 12, 2022
View reviewed changes
Comment threadpymbolic/mapper/substitutor.py
Comment on lines +59 to +63
class CachedSubstitutionMapper(CachedIdentityMapper,
SubstitutionMapper):
def __init__(self, subst_func):
CachedIdentityMapper.__init__(self)
SubstitutionMapper.__init__(self, subst_func)

@matthiasdienermatthiasdienerMay 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this actually return from the cache for map_variable, map_subscript, map_lookup?

@inducerinducerMay 13, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why wouldn't it?

@matthiasdienermatthiasdienerMay 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was just wondering because the SubstitutionMapper from which this is derived directly calls the IdentityMapper for these methods. But if I understood the MRO here correctly, this appears to not be an issue.

@inducerinducerMay 17, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

SubstitutionMapper from which this is derived directly calls the IdentityMapper for these methods.

Cache lookups happen in rec, but SubstitutionMapper doesn't override rec. I think this is OK, but maybe I'm missing something.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@inducerinducerAwaiting requested review from inducer
1 more reviewer
@matthiasdienermatthiasdienermatthiasdiener left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memoizing results of traversals

3 participants

@kaushikcfd@inducer@matthiasdiener