Use MD5 for creating cache keys #1396
Conversation
MapboxNavigation/Cache.swift
Outdated
| } | ||
|
|
||
| return String.init(key.hashValue) | ||
| return MD5(key) |
There was a problem hiding this comment.
This single line is the actual fix. As pointed out by @akitchen, there are some shortcuts String.hashValue takes some shortcuts and things in the middle of the string may not impact the hash.
MapboxNavigation/Cache.swift
Outdated
| if let keyAsURL = URL(string: key) { | ||
| return String.init(keyAsURL.lastPathComponent.hashValue) | ||
| return MD5(keyAsURL.lastPathComponent) |
There was a problem hiding this comment.
I guess I'm surprised to see us still keying off of lastPathComponent -- I thought this was part of what was biting us. (?)
There was a problem hiding this comment.
Not that I am aware of. It really only came down to two different strings having the same hashValue: #1391 (comment)
There was a problem hiding this comment.
I'm happy to remove this check though. I think creating an md5 of the key here would be okay.
MapboxNavigation/Cache.swift
Outdated
| } | ||
|
|
||
| return String.init(key.hashValue) | ||
| return MD5(key) |
There was a problem hiding this comment.
Do voice instructions fall through this code path? Asking for a friend...
There was a problem hiding this comment.
Yep, only voice instructions. Highway shields are keyed off of the url.
|
Maybe adjust the PR title before merging? |
bsudekum
changed the title
| func testCacheKeyForKey() { | ||
| let threeMileInstruction = "<speak><amazon:effect name=\"drc\"><prosody rate=\"1.08\">Continue on <say-as interpret-as=\"address\">I-80</say-as> East for 3 miles</prosody></amazon:effect></speak>" | ||
| let sixMileInstruction = "<speak><amazon:effect name=\"drc\"><prosody rate=\"1.08\">Continue on <say-as interpret-as=\"address\">I-80</say-as> East for 6 miles</prosody></amazon:effect></speak>" | ||
| XCTAssertNotEqual(cache.fileCache.cacheKeyForKey(threeMileInstruction), cache.fileCache.cacheKeyForKey(sixMileInstruction)) |
There was a problem hiding this comment.
Great test case, this is the one that nails it.
| measure { | ||
| for _ in 0...1000 { | ||
| _ = cache.fileCache.cacheKeyForKey(instructionTurn) | ||
| _ = cache.fileCache.cacheKeyForKey(instructionContinue) |
There was a problem hiding this comment.
Can you post times from master and this branch for comparison?
There was a problem hiding this comment.
Yeah, it was 0.001 seconds for the same test.
MapboxNavigation/MD5.swift
Outdated
|
|
||
| /* | ||
| * Convert a raw string to an array of little-endian words | ||
| * Characters >255 have their high-byte silently ignored. |
There was a problem hiding this comment.
We do care about the high byte. Add test cases for non-ASCII characters, all the way up to emoji like 🛡 (U+1F6E1). Although emoji would be somewhat contrived for an OSM-based data set, there are plenty of multibyte characters in OSM, especially in Chinese and Japanese.
There was a problem hiding this comment.
Looks like we’re no longer using this code, but a test case would still be good for peace of mind.
Beginning to fix #1391
As pointed out by @akitchen,
cacheKeyForKeyis most likely too naive.mapbox-navigation-ios/MapboxNavigation/Cache.swift
Lines 122 to 128 in d7f8c4d
/cc @mapbox/navigation-ios