Skyhight173/json: add cache to improve performance#2457
Skyhight173/json: add cache to improve performance#2457LegoBrainBiker wants to merge 10 commits into
Conversation
github-actions
Bot
added
the
pr: change existing extension
|
!format |
|
!format |
| const objectCache = {}; | ||
| function parse(string) { | ||
| return ( | ||
| objectCache[string] ?? |
There was a problem hiding this comment.
never write code like this. clever code is bad and unreadable
There was a problem hiding this comment.
is it acceptable without the timeout?
There was a problem hiding this comment.
i was referring to objectCache[string] ?? (objectCache[string] = JSON.parse(string)); which is very indicipherable to normal people
|
!format |
Fix setInterval usage to correctly clear the cache.
|
!format |
|
I don't think this should be merged. Caching isn't always good, and in some cases people may want to avoid it. If people do want caching:
This extension is currently unique precisely because it does not do caching. The main concern with caching is that it increases RAM usage. Especially with implementation like this, if project does a lot of operations synchronously (within "run without screen refresh" custom block), it may quite easy to run out of RAM and crash the browser tab on projects that previously ran fine. |
|
I gotta agree with Xeltalliv here, this should be closed in favor of the other JSON extension PRs |
anything that would use a significant amount of memory already wouldn't run "fine" the current implementation runs very slowly with I personally don't use the JSON extension, instead using my own object extension. I agree that the json extension is not useful in most projects, and could be replaced, but that's no reason not to make the existing json extension the best it can be. |
|
The only problem with changing the existing extension is that it would have to implement breaking changes |
this pull request has no breaking changes, to a user, it would work exactly the same but faster. |
The problem is it would only work faster for preexisting objects that are the exact same. Additionally it would hog RAM space like previously mentioned |
|
So there's not much speed gain for regular usage. |
preexisting objects normally need to be reparsed whenever multiple items of an array/object are read. additionally, if an existing array or object is modified or indexed, my changes also cache those, so if for example if you want to apply multiple operations to an object or array, it would only have to parse the initial object or array, not each time an item is modified. I cannot think of a single scenario which this would not speed up. |
|
Yes but that quickly builds up in ram. And in those cases it's typically a one-time-use |
it only lasts for a short time. |
|
usually yes but not necessarily always for example, a run w/o screen refresh would never be clearing its cache with the extension as-written there's some ways you could address that. a max size cap in some way (probably try to account for big JSON objects and such) might make it tolerable w/o making things slower even in the worst case i'd be chill with that kind of idea, done well |
6 participants
the json extension can be slow because it needs to parse and stringify each time a block is called. this pr adds a cache so that if the json was recently created by the project, it can be looked up without using JSON.parse.