Add an upper limit text size for element text caching #215
Conversation
schwartzmx
commented
Nov 18, 2024
schwartzmx
commented
- Adds an upper limit to reduce chance of storing large strings in the cache which can increase memory of the cache map
facebook-github-bot
added
the
CLA Signed
| @@ -448,6 +448,7 @@ export type ALElementTextOptions = Types.Options< | |||
| updateText?: <T extends ALElementText>(elementText: T, domSource: ALDOMTextSource) => void; | |||
| getText?: <T extends ALElementText>(elementTexts: T[]) => ALElementText; | |||
| enableElementTextCache?: boolean; | |||
| elementCacheTextLengthLimit?: number; | |||
There was a problem hiding this comment.
Do you think we should apply this limit only for caching, or in general for the elementText?
It might be useful to say, if text is large, probably our algorithm is not working correctly.
In that case, may be instead of just not caching, we can replace it with something very clear like: !$ELEMENT$TEXT$TOO$LARGE$! (something we know is very unlikely to exist in the application itself).
This way, we still get the perf benefit of large text.
With this implementation, we actually going to turn off caching for the worst offender, i.e. large text extraction!
There was a problem hiding this comment.
Yeah that is a good point and I was thinking about that as well.
The issue though is I didn't want to cause any havoc with any existing reliance on large text, but we could look at going with your suggested approach behind a flag.
- Adds an upper limit to reduce chance of storing large strings in the cache which can increase memory of the cache map
schwartzmx
force-pushed
the
textcache-textlimit
branch
from
910ec10 to
c378e1e
Compare
