-
Notifications
You must be signed in to change notification settings - Fork 87
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
Cross reference search speed improvements [v3] #424
Conversation
Changed to empty struct to reduce allocations.
Codecov Report
@@ Coverage Diff @@
## v3 #424 +/- ##
==========================================
- Coverage 59.56% 59.53% -0.03%
==========================================
Files 153 153
Lines 27683 27707 +24
==========================================
+ Hits 16489 16495 +6
- Misses 10807 10836 +29
+ Partials 387 376 -11
Continue to review full report at Codecov.
|
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.
The code looks good. Left a couple of minor observations.
pdf/core/parser.go
Outdated
parser.xrefs[objNum] = obj | ||
common.Log.Trace("entry: %+v", parser.xrefs[objNum]) | ||
parser.xrefs.ObjectMap[objNum] = obj | ||
common.Log.Trace("entry: %+v", parser.xrefs.ObjectMap[objNum]) |
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.
Maybe just print obj
instead of reading it from the map after adding it:
common.Log.Trace("entry: %+v", obj)
} | ||
|
||
i := sort.Search(len(parser.xrefs.sortedObjects), func(i int) bool { |
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.
This seems to be equivalent to:
for _, obj := range parser.xrefs.sortedObjects {
if obj.Offset >= offset {
return obj.Offset
}
}
return 0
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.
Yes, except sort.Search uses binary search which is very efficient for sorted data.
pdf/core/parser.go
Outdated
} | ||
|
||
// Sort by offset, descending. |
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.
Seems to be ascending offset order.
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.
Corrected.
…nidoc into v3-xref-perf-improvements
Two notable performance enhancements:
Changes required changing XrefTable to a struct to be able to add the cached list of objects for faster search.
The changes speed up passthrough benchmark test in the build server from 38-40seconds down to 15 seconds, so it's a pretty significant improvement.