Cache dependencies parsed from each individual import #4568
Conversation
|
What is the impact of this cache on session size? Or is it a shared cache for the entire application? |
|
The cache is shared for the entire application (field in |
Legioth
force-pushed
the
dependencyTreeCache
branch
from
7014fcd
to
0223ce8
Compare
| cache.put(node, dependencies); | ||
|
|
||
| synchronized (placeholder) { | ||
| placeholder.notifyAll(); |
There was a problem hiding this comment.
|
|
||
| private Collection<T> waitForDependencies(T node, Object placeholder) | ||
| throws InterruptedException { | ||
| synchronized (placeholder) { |
There was a problem hiding this comment.
| * @author Vaadin Ltd | ||
| * @since 1.0 | ||
| * | ||
| */ | ||
| @Deprecated | ||
| public class HtmlDependencyParser implements Serializable { |
There was a problem hiding this comment.
|
|
||
| public MockParser addResult(String path, int duration, | ||
| String... dependencies) { | ||
| return addResult(path, () -> Thread.sleep(duration), dependencies); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Reviewed 11 of 11 files at r1.
Reviewable status: 8 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 175 at r1 (raw file):
*/ public void clear() { cache.clear();
Should we for clearing keep track of parsing instances and then wait for any to complete before clearing or just have them stop before clearing?
Previously, dependency parsing results were only cached per component class. This means that if multiple components use the same import, that import would have been parsed multiple times. This patch adds a separate cache layer that keeps track of the imports from individual files, and then traverses that cache to collect all reachable imports. Reduces total TTFB for the first view in Beverage Buddy from 1.2 to 0.6 seconds and from 1.1 to 0.3 seconds for opening the dialog. Related to #4532.
Legioth
force-pushed
the
dependencyTreeCache
branch
from
0223ce8
to
ee8be9d
Compare
There was a problem hiding this comment.
Dismissed @vaadin-bot from a discussion.
Reviewable status: 7 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 81 at r1 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Either re-interrupt this method or rethrow the "InterruptedException".
Done.
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 82 at r1 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Anyone want to suggest what to throw, or should we just ignore sonar here?
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 114 at r1 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Nope. The observable effect is in the cache map.
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 130 at r1 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Nope.
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 175 at r1 (raw file):
Previously, caalador wrote…
Should we for clearing keep track of parsing instances and then wait for any to complete before clearing or just have them stop before clearing?
I don't think that would be worthwhile. The main reason for clearing this cache is if the file contents have been changed. In that situation, I'd say that it's quite fine that any processing done during the change will get potentially inconsistent results (since that's also what they'd get without the cache), as long as any new processing won't see any signs of the old file contents.
flow-server/src/main/java/com/vaadin/flow/component/internal/HtmlImportParser.java, line 108 at r1 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Move the "import" string literal on the left side of this string comparison.
Done.
flow-server/src/test/java/com/vaadin/flow/component/internal/DependencyTreeCacheTest.java, line 178 at r1 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Nope.
| // Restore interrupted state | ||
| Thread.currentThread().interrupt(); | ||
|
|
||
| throw new RuntimeException( |
There was a problem hiding this comment.
|
SonarQube analysis reported 19 issues
Watch the comments in this conversation to review them. Top 10 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Dismissed @vaadin-bot from 5 discussions.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 82 at r1 (raw file):
Previously, Legioth (Leif Åstrand) wrote…
Anyone want to suggest what to throw, or should we just ignore sonar here?
Feels like a ignore. Can't really come up with any descriptive exception name.
flow-server/src/main/java/com/vaadin/flow/component/internal/DependencyTreeCache.java, line 175 at r1 (raw file):
Previously, Legioth (Leif Åstrand) wrote…
I don't think that would be worthwhile. The main reason for clearing this cache is if the file contents have been changed. In that situation, I'd say that it's quite fine that any processing done during the change will get potentially inconsistent results (since that's also what they'd get without the cache), as long as any new processing won't see any signs of the old file contents.
👍
There was a problem hiding this comment.
Dismissed @vaadin-bot from 2 discussions.
Reviewable status: 3 unresolved discussions, 1 of 1 LGTMs obtained
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, 1 of 1 LGTMs obtained
Previously, dependency parsing results were only cached per component
class. This means that if multiple components use the same import, that
import would have been parsed multiple times.
This patch adds a separate cache layer that keeps track of the imports
from individual files, and then traverses that cache to collect all
reachable imports.
Reduces total TTFB for the first view in Beverage Buddy from 1.2 to 0.6
seconds and from 1.1 to 0.3 seconds for opening the dialog.
Related to #4532.
This change is