-
Notifications
You must be signed in to change notification settings - Fork 24
Allow the dynamic PDF-name cache to be cleared and bounded #730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -38,6 +38,48 @@ public class ASAtom implements Comparable<ASAtom> { | |||||||||||
| private static final Map<String, ASAtom> PREDEFINED_PDF_NAMES = Collections.synchronizedMap(new HashMap<>()); | ||||||||||||
| private static final Map<String, ASAtom> CACHED_PDF_NAMES = Collections.synchronizedMap(new HashMap<>()); | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Optional upper bound on the number of dynamically encountered PDF names kept in | ||||||||||||
| * {@link #CACHED_PDF_NAMES}. The cache is a process-wide memoization of names that are not one of | ||||||||||||
| * the predefined constants; it never evicts, so a stream of documents with many unique names keeps | ||||||||||||
| * it growing for the lifetime of the process. A value of {@code -1} (the default) keeps the | ||||||||||||
| * historical unbounded behaviour; a non-negative value stops adding new names once the cache holds | ||||||||||||
| * that many, in which case {@link #getASAtom(String)} still returns a correct atom, just an | ||||||||||||
| * uncached one. Equality of atoms is by value ({@link #equals(Object)}), so an uncached atom | ||||||||||||
| * behaves identically to a cached one. | ||||||||||||
| */ | ||||||||||||
| private static volatile int maxCachedNames = -1; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Sets the upper bound on the number of cached dynamic PDF names. A negative value removes the | ||||||||||||
| * bound (the default). | ||||||||||||
| * | ||||||||||||
| * @param max maximum number of cached dynamic names, or a negative value for no bound | ||||||||||||
| */ | ||||||||||||
| public static void setMaxCachedNames(int max) { | ||||||||||||
| maxCachedNames = max; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * @return the current upper bound on cached dynamic names, or {@code -1} if unbounded | ||||||||||||
| */ | ||||||||||||
| public static int getMaxCachedNames() { | ||||||||||||
| return maxCachedNames; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Clears the cache of dynamically encountered PDF names. The predefined name constants are not | ||||||||||||
| * affected. A long-running process that validates untrusted documents can call this between jobs to | ||||||||||||
| * release names accumulated from earlier documents. | ||||||||||||
| */ | ||||||||||||
| public static void clearCache() { | ||||||||||||
| CACHED_PDF_NAMES.clear(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private static boolean isCacheBelowLimit() { | ||||||||||||
| return maxCachedNames < 0 || CACHED_PDF_NAMES.size() < maxCachedNames; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // 3 | ||||||||||||
| public static final ASAtom key3D = new ASAtom("3D"); | ||||||||||||
| public static final ASAtom key3DD = new ASAtom("3DD"); | ||||||||||||
|
|
@@ -696,7 +738,7 @@ private ASAtom(String value, boolean predefinedValue) { | |||||||||||
| if (predefinedValue) { | ||||||||||||
| PREDEFINED_PDF_NAMES.put(value, this); | ||||||||||||
| } else { | ||||||||||||
| if (!CACHED_PDF_NAMES.containsKey(value)) { | ||||||||||||
| if (isCacheBelowLimit() && !CACHED_PDF_NAMES.containsKey(value)) { | ||||||||||||
| CACHED_PDF_NAMES.put(value, this); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
@@ -719,9 +761,8 @@ public static ASAtom getASAtom(String value) { | |||||||||||
| if (CACHED_PDF_NAMES.containsKey(value)) { | ||||||||||||
| return CACHED_PDF_NAMES.get(value); | ||||||||||||
|
Comment on lines
761
to
762
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
rg -n -C 5 'ASAtom\.clearCache\s*\(|import static org\.verapdf\.as\.ASAtom\.clearCache' src --glob '*.java'Repository: veraPDF/veraPDF-parser Length of output: 160 🏁 Script executed: #!/bin/bash
set -o pipefail
printf '%s\n' '--- ASAtom declarations and methods ---'
rg -n -C 12 'CACHED_PDF_NAMES|clearCache|getASAtom|setMaxCachedNames|getMaxCachedNames' src/main/java/org/verapdf/as/ASAtom.java
printf '%s\n' '--- COSName bindings ---'
rg -n -C 10 'ASAtom\.getASAtom|class COSName|COSName\s*\(' src/main/java --glob '*.java'
printf '%s\n' '--- all clearCache references ---'
rg -n -C 4 'clearCache\s*\(' . --glob '*.java' --glob '*.md' --glob '*.xml' --glob '*.properties' --glob '*.yml' --glob '*.yaml' || trueRepository: veraPDF/veraPDF-parser Length of output: 43359 Read the cached atom with one map lookup.
🐛 Suggested fix- if (CACHED_PDF_NAMES.containsKey(value)) {
- return CACHED_PDF_NAMES.get(value);
+ ASAtom cached = CACHED_PDF_NAMES.get(value);
+ if (cached != null) {
+ return cached;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| } | ||||||||||||
| ASAtom result = new ASAtom(value, false); | ||||||||||||
| CACHED_PDF_NAMES.put(value, result); | ||||||||||||
| return result; | ||||||||||||
| // The constructor is the single caching point; it honours the configured cache limit. | ||||||||||||
| return new ASAtom(value, false); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: veraPDF/veraPDF-parser
Length of output: 41447
🏁 Script executed:
Repository: veraPDF/veraPDF-parser
Length of output: 41257
🏁 Script executed:
trueRepository: veraPDF/veraPDF-parser
Length of output: 160
Make the cache limit check and insertion atomic.
CACHED_PDF_NAMESsynchronizes individual map operations, but the size check and insertion are separate. Concurrent calls togetASAtom(String)can insert multiple distinct names after observing the same available capacity. The cache can therefore retain more entries thanmaxCachedNames, although the overshoot is limited by the number of racing constructors.Suggested fix
} else { - if (isCacheBelowLimit() && !CACHED_PDF_NAMES.containsKey(value)) { - CACHED_PDF_NAMES.put(value, this); + synchronized (CACHED_PDF_NAMES) { + if (isCacheBelowLimit() && !CACHED_PDF_NAMES.containsKey(value)) { + CACHED_PDF_NAMES.put(value, this); + } } }🤖 Prompt for AI Agents