Page MenuHomeWildfire Games
Differential D3615

Predictable Replay cache rebuild
ClosedPublic
Actions

Authored by nwtour on Feb 28 2021, 10:38 PM.

Details

Reviewers
wraitii
Commits
rP25026: Update the replay cache to consider modification time.
Summary

After updating the version, the list of replays disappeared .

Expected:
cp -r ~/.local/share/0ad/replays/0.0.24 ~/.local/share/0ad/replays/0.0.25
sed -i 's/0.0.24/0.0.25/' ~/.local/share/0ad/replays/0.0.25/2021-02-27_0003/*

But the list remained empty

Issue #1: 0ad doesn't refresh the list after clicking the button "Rebuild Cache". You must exit and re-enter the section "Replays"

Issue #2: Replay cache store only size of replays. And update version does not affect cache update (size not changed).
mtime removed here https://code.wildfiregames.com/D112 and that's a bad idea
Cache is consistent with only two parameters (date and size)

Patch update menu after rebuild and return internal value _MTimeInCache not used by the game and serving only to check the consistency of the cache

First click on "Rebuild Cache" in console:
Loading 258 cached replays, removed 258 outdated entries, loaded 258 new entries
Second run:
Loading 258 cached replays, removed 0 outdated entries, loaded 0 new entries
Cache work

Test Plan

.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nwtour requested review of this revision.Feb 28 2021, 10:38 PM
nwtour created this revision.
vladislavbelov added a subscriber: vladislavbelov.Feb 28 2021, 10:47 PM

Update years of the modified files.

source/ps/VisualReplay.cpp
140 ↗(On Diff #16151)

The name should follow the style.

142 ↗(On Diff #16151)

A tuple or a struct instead of sum.

Imarok added a subscriber: Imarok.Feb 28 2021, 11:12 PM
nwtour added inline comments.Mar 1 2021, 8:14 PM
source/ps/VisualReplay.cpp
140 ↗(On Diff #16151)

The name should follow the style.

If possible an example. If I understand correctly, mtime was removed as part to solve problem 2038. If it's just a "MTime", another programmer will decide to use this value in the game, but I'm not sure if this is a good idea.

wraitii added a subscriber: wraitii.Mar 1 2021, 8:29 PM
wraitii added inline comments.
source/ps/VisualReplay.cpp
140 ↗(On Diff #16151)

Vlad meant 'no underscore in front'

nwtour updated this revision to Diff 16183.Mar 1 2021, 9:21 PM

Update year
Name "_fileMTimeInCache" renamed to "fileTimeInCache"
std::pair -> std::tuple

Smoke test passed:
Loading 262 cached replays, removed 0 outdated entries, loaded 0 new entries
...
sed -i 's/0.0.24/0.0.25/' ~/.local/share/0ad/replays/0.0.25/2021-02-11_0003/*
...
Loading 262 cached replays, removed 1 outdated entries, loaded 1 new entries
and updated in GUI

nwtour updated this revision to Diff 16184.Mar 1 2021, 9:23 PM

fixed typo

vladislavbelov added inline comments.Mar 1 2021, 9:26 PM
source/ps/VisualReplay.cpp
415 ↗(On Diff #16183)

Missed.

140 ↗(On Diff #16151)

Not exactly, fileSize and directory are put in the same object but they don't have InCache suffix. So it seems inconsistent. I'd prefer to have for all or to not have for all. But if the object is used only for cache then the suffix is redundant.

nwtour updated this revision to Diff 16189.Mar 1 2021, 9:57 PM

fileTimeInCache -> fileMTime

vladislavbelov added inline comments.Mar 1 2021, 10:02 PM
source/ps/VisualReplay.cpp
119 ↗(On Diff #16189)

I believe that double is used only for conversion to JS values, but comparing doubles is kind of dangerous.

nwtour updated this revision to Diff 16196.Mar 2 2021, 12:44 AM

compare size as off_t, mtime as u64

nwtour added inline comments.Mar 2 2021, 12:45 AM
source/ps/VisualReplay.cpp
119 ↗(On Diff #16189)

I believe that double is used only for conversion to JS values, but comparing doubles is kind of dangerous.

Yes. My mistake. Fixed

wraitii added a comment.Mar 4 2021, 2:13 PM

I think both issues you're fixing are technically not "part of the spec", let me explain:

Issue #1: 0ad doesn't refresh the list after clicking the button "Rebuild Cache". You must exit and re-enter the section "Replays"

It does if you delete from in-game. Changing files externally is not really well supported in 0 A.D. in general (exception: hot loading).

Issue #2: Replay cache store only size of replays. And update version does not affect cache update (size not changed).

Changing replay files themselves is also not something that's really supposed to happen, so not a huge surprise there either.

That being said, I see no harm in improving this. I'll merge this, could you:

  • change typedef to using for replayMapCache
  • Instead of calling displayReplayList in reloadCache, do so in the XML file calling reloadCache.
nwtour added a comment.Mar 4 2021, 5:29 PM
In D3615#158773, @wraitii wrote:

It does if you delete from in-game. Changing files externally is not really well supported in 0 A.D. in general (exception: hot loading).

I checked: if I delete the replay from the game, I don't need to press the button "Rebuild Cache", the cache entry will be update automatically.
IMHO this means that the button is used for emergency/non-standart situations and should work as it should for the Cache: update the changed files and immediately show the results of the updated cache

This is not about hot-modifying the cache files. The game starts already with an "non-valid" cache...

I'll merge this, could you...

ok

Imarok added a comment.EditedMar 4 2021, 5:50 PM
In D3615#158814, @nwtour wrote:

IMHO this means that the button is used for emergency/non-standart situations and should work as it should for the Cache: update the changed files and immediately show the results of the updated cache

I think you have a point here.

Edit: If you are interested in the discussion about the implementation of the cache see D39.

nwtour updated this revision to Diff 16292.Mar 4 2021, 7:05 PM

typedef -> using
displayReplayList(): replay_menu.js -> replay_menu.xml

wraitii accepted this revision.Mar 7 2021, 9:47 AM
This revision is now accepted and ready to land.Mar 7 2021, 9:47 AM
Closed by commit rP25026: Update the replay cache to consider modification time. (authored by wraitii). · Explain WhyMar 7 2021, 9:56 AM
This revision was automatically updated to reflect the committed changes.
wraitii added a commit: rP25026: Update the replay cache to consider modification time..
Owners added a subscriber: Restricted Owners Package.Mar 7 2021, 9:56 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
replaymenu/
2 lines
source/
ps/
16 lines

Diff 16355

View Options

ps/trunk/binaries/data/mods/public/gui/replaymenu/replay_menu.xml

Loading...
View Options

ps/trunk/source/ps/VisualReplay.cpp

Loading...
Wildfire Games · Phabricator