Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Removes the binary reader's SymbolToken cache, which provided a minor optimization for certain cases but a major regression for others. #756

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Removes the binary reader's SymbolToken cache, which provided a minor optimization for certain cases but a major regression for others. #756

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 5 additions & 28 deletions src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ class IonReaderContinuableApplicationBinary extends IonReaderContinuableCoreBina
// The first lowest local symbol ID in the symbol table.
private int firstLocalSymbolId = imports.getMaxId() + 1;

// A map of symbol ID to SymbolToken representation. Because most use cases only require symbol text, this
// is used only if necessary to avoid imposing the extra expense on all symbol lookups.
private List<SymbolToken> symbolTokensById = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution would be to change this to a Map<Int, SymbolToken> and then lazily populate it as symbols are requested. This means that (a) the size of the cache is determined by the number of unique SymbolTokens that are requested, and (b) each SymbolToken is allocated only once. However, it does incur the cost of looking something up in a Map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I called this out in the PR description too. I'd prefer to remove the cache until it's clear that there is a use case that demands it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I don't know how I missed that.


// The cached SymbolTable representation of the current local symbol table. Invalidated whenever a local
// symbol table is encountered in the stream.
private SymbolTable cachedReadOnlySymbolTable = null;
Expand Down Expand Up @@ -448,9 +444,6 @@ private void resetSymbolTable() {
Arrays.fill(symbols, 0, localSymbolMaxOffset + 1, null);
localSymbolMaxOffset = -1;
cachedReadOnlySymbolTable = null;
if (symbolTokensById != null) {
symbolTokensById.clear();
}
}

/**
Expand Down Expand Up @@ -479,9 +472,6 @@ protected void restoreSymbolTable(SymbolTable symbolTable) {
localSymbolMaxOffset = snapshot.maxId - firstLocalSymbolId;
// Note: because `symbols` only grows, `snapshot.listView` will always fit within `symbols`.
System.arraycopy(snapshot.idToText, 0, symbols, 0, snapshot.idToText.length);
if (symbolTokensById != null) {
symbolTokensById.clear();
}
} else {
// Note: this will only happen when `symbolTable` is the system symbol table.
resetSymbolTable();
Expand Down Expand Up @@ -568,28 +558,15 @@ String getSymbol(int sid) {
*/
private SymbolToken getSymbolToken(int sid) {
int symbolTableSize = localSymbolMaxOffset + firstLocalSymbolId + 1; // +1 because the max ID is 0-indexed.
if (symbolTokensById == null) {
symbolTokensById = new ArrayList<>(symbolTableSize);
}
if (symbolTokensById.size() < symbolTableSize) {
for (int i = symbolTokensById.size(); i < symbolTableSize; i++) {
symbolTokensById.add(null);
}
}
if (sid >= symbolTableSize) {
throw new UnknownSymbolException(sid);
}
SymbolToken token = symbolTokensById.get(sid);
if (token == null) {
String text = getSymbolString(sid, imports, symbols);
if (text == null && sid >= firstLocalSymbolId) {
// All symbols with unknown text in the local symbol range are equivalent to symbol zero.
sid = 0;
}
token = new SymbolTokenImpl(text, sid);
symbolTokensById.set(sid, token);
String text = getSymbolString(sid, imports, symbols);
if (text == null && sid >= firstLocalSymbolId) {
// All symbols with unknown text in the local symbol range are equivalent to symbol zero.
sid = 0;
}
return token;
return new SymbolTokenImpl(text, sid);
}

/**
Expand Down
Loading