Repository navigation
Use the whole AST as key cache (close #708) - #710
Conversation
Most of the data in the AST is being retained anyway so trying to optimize it with .hash doesn't buy much in practice (I've tested it on my projects). The other problem with the cache is it can grow indefinitely, in theory. If an app produces dynamic queries using combines/select etc the memory for AST and interim structs will be retained but it's a separate problem, the current implementation is vulnerable just as well.
timriley
left a comment
timriley
left a comment
There was a problem hiding this comment.
Thanks @flash-gordon! I wasn't confident enough to know whether retaining the objects as the keys would be an issue, but if you're comfortable with this, it looks good to me 👍🏼
|
Is it worth filing an issue about the unbounded cache? |
|
@timriley, conditionally, yes. The problem has to be justified against a possible solution. An agent can draft the approach so it's easier to decide whether we want to trade the added complexity for the corner case. What I found today is that the existence of the cache is 100% justified. Building a mapper for a complex query can easily take a few ms |
|
(FYI, I switched the base branch to |
|
Looks like this fixed the rom-factory flakes! (After I updated the branches over in that Gemfile: hanakai-rb/rom-factory#101). Thanks @flash-gordon! |
2 participants
Most of the data in the AST is retained anyway, so optimizing it with .hash doesn't buy much in practice (I've tested it on my projects). The other problem with the cache is that it can grow indefinitely, in theory. If an app produces dynamic queries using combines/select, etc the memory for AST and interim structs will be retained, but it's a separate problem; the current implementation is vulnerable just as well.