Opened 9 years ago
Closed 7 years ago
#3433 closed defect (fixed)
[PATCH] Fix replay menu loading time by using a cache file
Reported by: | elexis | Owned by: | Imarok |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 22 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
This problem was reported in ticket:3258#comment:15
Problem:
If you open the replay menu, it will read all replay files it finds in the sim_log
directory.
While the parsing in C++ is quite optimized (only reads the first and last 5 or so lines) and also the JS & GUI handling is fast enough (half a second for 2500 replays), having to open these files the first time can take up to a minute. This is because the files are scattered around on the harddrive and it needs some mechanical work to locate the files.
After that the files are loaded into the HDD cache and this process only takes milliseconds.
Reproduce: On linux you can drop the HDD cache by executing:
echo 3 | sudo tee /proc/sys/vm/drop_caches
Solution: The correct solution to address this disk I/O problem is to have a custom cache file. This way files will be read only once, ever. (Immediately after the game or when opening the menu and finding new files).
Attachments (13)
Change History (46)
comment:1 by , 9 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
comment:3 by , 8 years ago
Owner: | set to |
---|
by , 8 years ago
Attachment: | 3433_replay_cache_v1.patch added |
---|
Implement a replay data cache. When opening the replay menu, check for added and removed replays and update the cache
comment:4 by , 8 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 21 |
Summary: | Fix replay menu loading time by using a cache file → [PATCH] Fix replay menu loading time by using a cache file |
by , 8 years ago
Attachment: | 3433_replay_cache_v1.3.patch added |
---|
Add replay to cache directly after playing a game. Delete replay from the cache when deleting a replay with the replaymenu gui. Add abutton for reloading the cache totally. Some minor fixes/style changes. (VS seems to have changed some line breaks. Don't know how to revert that :/)
comment:5 by , 8 years ago
You can go File -> Advanced Save Options and then set the line endings to Unix.
comment:6 by , 8 years ago
JSInterface_VisualReplay.cpp had inconsistent line breaks. VS changed some line breaks, so that the whole file uses the same line breaks.
comment:7 by , 8 years ago
Keywords: | rfc added; review removed |
---|
Move tickets from the review queue to the rfc one.
by , 8 years ago
Attachment: | 3433_replay_cache_v1.6_wip.patch added |
---|
by , 8 years ago
Attachment: | 3433_replay_cache_v1.6.patch added |
---|
Remove the reloadCache functionality. Compare fileName and fileSize to decide if the file changed. Some minor fixes
comment:8 by , 8 years ago
- *pCxPrivate->pScriptInterface instead of *(pCxPrivate->pScriptInterface), prior. of -> bigger than *
- JS_SetElement(cx, replays, i++, replay); > JS_SetElement(cx, replays, ++i, replay);, as for loops?
How often function GetReplays has been called? Because map isn't fast, you could replace it with vector if all filenames are differents, or unordered_map if not.
The rest of the patch looks good.
P.S. would be interesting too see the replays loading in the profiler.
by , 8 years ago
Attachment: | 3433_replay_cache_v1.6_vector.patch added |
---|
Use a vector instead of a map
comment:9 by , 8 years ago
Performance comparison (Intel Core i3 2.1GHz, 4GB, SSD):
2048 replays:
Vector (Cached) Avg:
1.299s
Map (Cached) Avg:
1,271s
Unordered Map (Cached) Avg:
1,250s
Vector (Not Cached) Avg:
2.374s
Map (Not Cached) Avg:
2,421s
Unordered Map (Not Cached) Avg:
2,357s
Hash for CStr/unordered_map:
struct KeyHasher { std::size_t operator()(const CStr& key) const { return std::hash<std::string>()(key.c_str()); } };
comment:10 by , 8 years ago
source/ps/VisualReplay.cpp:104: JS::RootedValue file(cx);
is unused, should be removed.
follow-up: 13 comment:11 by , 8 years ago
Probably, need to add to zip the cache file, because for me: cache.json - 7.7MB, cache.json.zip - 92.6KB.
follow-up: 14 comment:13 by , 8 years ago
Replying to vladislavbelov:
Probably, need to add to zip the cache file, because for me: cache.json - 7.7MB, cache.json.zip - 92.6KB.
I think zipping is not really needed, as 7,7MB with 2048 replays isn't that much. (It will also slow down the GetReplays function)
comment:14 by , 8 years ago
Replying to Imarok:
Replying to vladislavbelov:
Probably, need to add to zip the cache file, because for me: cache.json - 7.7MB, cache.json.zip - 92.6KB.
I think zipping is not really needed, as 7,7MB with 2048 replays isn't that much. (It will also slow down the GetReplays function)
Over 2 seconds loading time is too slow for 2048 list items. Time reading of SIZE bigger than time of ZIPPED_SIZE + UNZIP.
comment:15 by , 8 years ago
Milestone: | Alpha 21 → Alpha 22 |
---|
by , 8 years ago
Attachment: | 3433_replay_cache_v1.7.patch added |
---|
Readd the reload button. Only compare fileSize when reloading the cache, because that is very time-consuming. Include replays without data into the cache.
follow-up: 18 comment:17 by , 7 years ago
JS_GetArrayLength(cx, cachedReplaysObject, &cacheLength)
could make the cacheLength to be undefined. Use condition:
if (!JS_GetArrayLength(cx, cachedReplaysObject, &cacheLength)) { JS_ReportError(cx, "Fail ... "); // ... }
The same thing for the JS_GetElement
.
follow-up: 19 comment:18 by , 7 years ago
Replying to vladislavbelov:
JS_GetArrayLength(cx, cachedReplaysObject, &cacheLength)
could make the cacheLength to be undefined. Use condition:if (!JS_GetArrayLength(cx, cachedReplaysObject, &cacheLength)) { JS_ReportError(cx, "Fail ... "); // ... }The same thing for the
JS_GetElement
.
In which case should JS_GetArrayLength
or JS_GetArrayLength
return undefined?
follow-up: 20 comment:19 by , 7 years ago
Replying to Imarok:
In which case should
JS_GetArrayLength
orJS_GetArrayLength
return undefined?
In case we got not an array object. It's the bad practise to not use a check with the script data. Look at source/gui/scripting/JSInterface_IGUIObject.cpp.
follow-up: 21 comment:20 by , 7 years ago
Replying to vladislavbelov:
Replying to Imarok:
In which case should
JS_GetArrayLength
orJS_GetArrayLength
return undefined?In case we got not an array object. It's the bad practise to not use a check with the script data. Look at source/gui/scripting/JSInterface_IGUIObject.cpp.
In that case the command after that already throws an JS_ReportError...
comment:21 by , 7 years ago
Replying to Imarok:
In that case the command after that already throws an JS_ReportError...
It's not a clean way to catch error in one function by another. Also the function is still working, even a replay is broken, but we'd print an error and exit or clean a cached data.
follow-up: 23 comment:22 by , 7 years ago
but JS_ReportError
crashes the game
though I agree there should be a check, if the cache file has a valid content
follow-up: 24 comment:23 by , 7 years ago
Replying to Imarok:
but
JS_ReportError
crashes the gamethough I agree there should be a check, if the cache file has a valid content
For me (windows) it doesn't crash, just print an error in the console.
follow-up: 25 comment:24 by , 7 years ago
Replying to vladislavbelov:
Replying to Imarok:
but
JS_ReportError
crashes the gamethough I agree there should be a check, if the cache file has a valid content
For me (windows) it doesn't crash, just print an error in the console.
Strange, I tested it again and it didn't crash, but just printed an error.
But it seems like JS_ReportError returns the function. So I think it would be better to just print the error with LOGERROR
and replace the invalid cache.
comment:25 by , 7 years ago
Replying to Imarok:
Strange, I tested it again and it didn't crash, but just printed an error. But it seems like JS_ReportError returns the function. So I think it would be better to just print the error with
LOGERROR
and replace the invalid cache.
Yeah, i.e. there's just JS_ReportError(...); return false;
in the JSInterface_IGUIObject.cpp. I'm not sure what should be there: LOGERROR or JS_ReportError (probably it sets some flags in the SM, and uses internal report handling):
// JS_ReportError by itself doesn't seem to set a JS-style exception, and so // script callers will be unable to catch anything. So use JS_SetPendingException // to make sure there really is a script-level exception. But just set it to undefined // because there's not much value yet in throwing a real exception object.
And:
/* * Report an exception represented by the sprintf-like conversion of format * and its arguments. This exception message string is passed to a pre-set * JSErrorReporter function (set by JS_SetErrorReporter). */
by , 7 years ago
Attachment: | 3433_replay_cache_v1.8.patch added |
---|
new functions ReadCacheFile
and StoreCacheFile
, assert JS_GetArrayLength
and JS_GetArrayLength
won't fail by checking if the cache is an array, check if the cache file is valid json
comment:26 by , 7 years ago
Keywords: | review added; rfc removed |
---|
follow-up: 28 comment:27 by , 7 years ago
Notes:
- Why this, looks weird?
std::istream* cacheStream = new std::ifstream(cacheFileName.string8().c_str()); CStr cacheStr((std::istreambuf_iterator<char>(*cacheStream)), std::istreambuf_iterator<char>()); SAFE_DELETE(cacheStream);
Why not this (ifstream is derived from istream through some things, so this iterator still works)?
std::ifstream cacheStream(cacheFileName.string8().c_str()); CStr cacheStr((std::istreambuf_iterator<char>(cacheStream)), std::istreambuf_iterator<char>()); if (cacheStream.is_open()) cacheStream.close();
- The
scriptInterface.GetProperty(replay, "directory", fileName)
returnfalse
if there was any error, but you code doesn't check that, and still works, even thedirectory
is invalid, empty I think.
- Why break on
if (SDL_QuitRequested())
and not the return?
- No needs to have
u32 i = 0;
as global (VisualReplay.cpp:243), move to the loopfor (u32 j = 0, i = 0; j < replaysLength; ++j)
.
- I'm not sure, just wondering, maybe use the same name style in the
scriptInterface.SetProperty(replayData, "fileSize", (u32)fileSize)
as other attributes:filesize
orfile_size
.
comment:28 by , 7 years ago
Replying to vladislavbelov: 1: fixed 2: as you said, not really an issue 3: fixed 4: I think that looks ugly ;) 5: CodingConventions say it should be lowerCamelCase
comment:29 by , 7 years ago
Thanks for adding the helper function to remove duplicate code.
- Would only occur if someone modifies the file. The could would be more clean checking this case and avoiding the garbage entries in the filemap.
- Not so fast, reconsider.
The OsPath
resolving bug #4320 should be considered. OsString
should likely be prefered over string8
(which is a lossy alias for utf8_from_wstring
as mentioned in path.h
).
comment:31 by , 7 years ago
Description: | modified (diff) |
---|---|
Milestone: | Work In Progress → Alpha 22 |
Since the issue is fixed what I found at last feature freeze day, hopefully we can make it in time this time -.-
That could also be used for maps loading, which can take some time too.