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 elexis)

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)

3433_replay_cache_v1.patch (4.0 KB ) - added by Imarok 8 years ago.
Implement a replay data cache. When opening the replay menu, check for added and removed replays and update the cache
3433_replay_cache_v1.1.patch (4.0 KB ) - added by Imarok 8 years ago.
Using .json instead of .txt
3433_replay_cache_v1.2.patch (3.9 KB ) - added by Imarok 8 years ago.
Some style fixes
3433_replay_cache_v1.3.patch (16.4 KB ) - added by Imarok 8 years ago.
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 :/)
3433_replay_cache_v1.5.patch (11.8 KB ) - added by Imarok 8 years ago.
rebased after r18561
3433_replay_cache_v1.6_wip.patch (13.0 KB ) - added by Imarok 8 years ago.
3433_replay_cache_v1.6.patch (10.0 KB ) - added by Imarok 8 years ago.
Remove the reloadCache functionality. Compare fileName and fileSize to decide if the file changed. Some minor fixes
3433_replay_cache_v1.6_vector.patch (10.2 KB ) - added by Imarok 8 years ago.
Use a vector instead of a map
3433_replay_cache_v1.7.patch (15.9 KB ) - added by Imarok 7 years ago.
Readd the reload button. Only compare fileSize when reloading the cache, because that is very time-consuming. Include replays without data into the cache.
3433_replay_cache_v1.8.patch (16.7 KB ) - added by Imarok 7 years ago.
new functions ReadCacheFileand 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
3433_replay_cache_v1.8.1patch (16.7 KB ) - added by Imarok 7 years ago.
see above comment
3433_replay_cache_v1.8.1.patch (16.7 KB ) - added by Imarok 7 years ago.
right filename
3433_replay_cache_v1.8.2.patch (16.7 KB ) - added by Imarok 7 years ago.
Move i into the for loop head

Download all attachments as: .zip

Change History (46)

comment:1 by elexis, 9 years ago

Description: modified (diff)

comment:2 by Itms, 8 years ago

That could also be used for maps loading, which can take some time too.

comment:3 by Imarok, 8 years ago

Owner: set to Imarok

by Imarok, 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 Imarok, 8 years ago

Keywords: review patch added
Milestone: BacklogAlpha 21
Summary: Fix replay menu loading time by using a cache file[PATCH] Fix replay menu loading time by using a cache file

by Imarok, 8 years ago

Using .json instead of .txt

by Imarok, 8 years ago

Some style fixes

by Imarok, 8 years ago

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 Sandarac, 8 years ago

You can go File -> Advanced Save Options and then set the line endings to Unix.

comment:6 by Imarok, 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.

by Imarok, 8 years ago

rebased after r18561

comment:7 by Itms, 8 years ago

Keywords: rfc added; review removed

Move tickets from the review queue to the rfc one.

by Imarok, 8 years ago

by Imarok, 8 years ago

Remove the reloadCache functionality. Compare fileName and fileSize to decide if the file changed. Some minor fixes

comment:8 by Vladislav Belov, 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 Imarok, 8 years ago

Use a vector instead of a map

comment:9 by Vladislav Belov, 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());
	}
};
Last edited 8 years ago by Vladislav Belov (previous) (diff)

comment:10 by Vladislav Belov, 8 years ago

source/ps/VisualReplay.cpp:104: JS::RootedValue file(cx); is unused, should be removed.

comment:11 by Vladislav Belov, 8 years ago

Probably, need to add to zip the cache file, because for me: cache.json - 7.7MB, cache.json.zip - 92.6KB.

comment:12 by elexis, 8 years ago

See SavedGame.cpp for an example

in reply to:  11 ; comment:13 by Imarok, 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)

in reply to:  13 comment:14 by Vladislav Belov, 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 elexis, 8 years ago

Milestone: Alpha 21Alpha 22

by Imarok, 7 years ago

Readd the reload button. Only compare fileSize when reloading the cache, because that is very time-consuming. Include replays without data into the cache.

comment:16 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:17 by Vladislav Belov, 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.

Last edited 7 years ago by Vladislav Belov (previous) (diff)

in reply to:  17 ; comment:18 by Imarok, 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?

in reply to:  18 ; comment:19 by Vladislav Belov, 7 years ago

Replying to Imarok:

In which case should JS_GetArrayLength or JS_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 reply to:  19 ; comment:20 by Imarok, 7 years ago

Replying to vladislavbelov:

Replying to Imarok:

In which case should JS_GetArrayLength or JS_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...

in reply to:  20 comment:21 by Vladislav Belov, 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.

Last edited 7 years ago by Vladislav Belov (previous) (diff)

comment:22 by Imarok, 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

in reply to:  22 ; comment:23 by Vladislav Belov, 7 years ago

Replying to Imarok:

but JS_ReportError crashes the game

though 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.

in reply to:  23 ; comment:24 by Imarok, 7 years ago

Replying to vladislavbelov:

Replying to Imarok:

but JS_ReportError crashes the game

though 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.

in reply to:  24 comment:25 by Vladislav Belov, 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).
 */
Last edited 7 years ago by Vladislav Belov (previous) (diff)

by Imarok, 7 years ago

new functions ReadCacheFileand 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 Imarok, 7 years ago

Keywords: review added; rfc removed

comment:27 by Vladislav Belov, 7 years ago

Notes:

  1. 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();
  1. The scriptInterface.GetProperty(replay, "directory", fileName) return false if there was any error, but you code doesn't check that, and still works, even the directory is invalid, empty I think.
  1. Why break on if (SDL_QuitRequested()) and not the return?
  1. No needs to have u32 i = 0; as global (VisualReplay.cpp:243), move to the loop for (u32 j = 0, i = 0; j < replaysLength; ++j).
  1. I'm not sure, just wondering, maybe use the same name style in the scriptInterface.SetProperty(replayData, "fileSize", (u32)fileSize) as other attributes: filesize or file_size.
Last edited 7 years ago by Vladislav Belov (previous) (diff)

in reply to:  27 comment:28 by Imarok, 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

by Imarok, 7 years ago

see above comment

by Imarok, 7 years ago

right filename

by Imarok, 7 years ago

Move i into the for loop head

comment:29 by elexis, 7 years ago

Thanks for adding the helper function to remove duplicate code.

  1. 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.
  2. 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:30 by Imarok, 7 years ago

Keywords: review removed

comment:31 by elexis, 7 years ago

Description: modified (diff)
Milestone: Work In ProgressAlpha 22

Since the issue is fixed what I found at last feature freeze day, hopefully we can make it in time this time -.-

comment:32 by Imarok, 7 years ago

still want to do that Vfs change before?

comment:33 by Imarok, 7 years ago

Resolution: fixed
Status: newclosed

In 19674:

Fix replay menu loading time by using a cache file

Reviewed by: elexis
Fixes #3433
Differential Revision: https://code.wildfiregames.com/D39

Note: See TracTickets for help on using tickets.