Skip to content

EC: skip unchanged files in INC_UPDATE for big-library performance - #727

Merged
mrjimenez merged 8 commits into
amule-project:masterfrom
got3nks:feat/ec-skip-unchanged-files
May 26, 2026
Merged

EC: skip unchanged files in INC_UPDATE for big-library performance#727
mrjimenez merged 8 commits into
amule-project:masterfrom
got3nks:feat/ec-skip-unchanged-files

Conversation

@got3nks

@got3nks got3nks commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Get_EC_Response_GetUpdate currently iterates every file in the encoder map on every INC_UPDATE request, constructs a fresh CEC_SharedFile_Tag / CEC_PartFile_Tag for each one, and lets the per-connection CObjTagMap diff inside the constructor decide which fields to emit. On a 91 k-file shareset that's 91 k constructor invocations per cycle, with most of the work thrown away because the file's exported fields didn't change. Profiling (#713) showed this is the dominant remaining EC dispatch cost after #725.

This PR adds a per-file change-generation counter that flips on every field mutation, and skips files whose generation hasn't advanced since the last response for that connection. For typical seedbox workloads where only a few hundred files of tens of thousands are actively transferring at any moment, per-cycle iteration drops from O(N) to O(files-actually-changed) — typically a 99 %+ reduction.

Scope: INC_UPDATE consumers only — amulegui and amuleweb. amulecmd is unaffected because amulecmd always issues FULL GET_SHARED_FILES, not INC_UPDATE. The amulecmd FULL-response path is a separate optimisation target if needed.

Three commits for reviewability

  1. Infrastructure — atomic per-file m_ecGen, process-wide atomic s_globalEcGen, MarkECChanged() method. No callers, no behavior change.
  2. HooksMarkECChanged() at every storage primitive that writes a field exported by CEC_SharedFile_Tag / CEC_PartFile_Tag:
    • CFileStatistic::AddRequest / AddAccepted / AddTransferred
    • CKnownFile::AddUploadingClient / RemoveUploadingClient
    • CKnownFile::SetFileName / SetFilePath / SetUpPriority / SetAutoUpPriority
    • CKnownFile::SetFileCommentRating
    • CKnownFile::UpdatePartsInfo
    • AICH master-hash setters (KnownFile.cpp / PartFile.cpp / amule.cpp hashing-task completion)
    • CKnownFile::Init so a freshly-constructed file is always above any existing threshold
    • CPartFile::Process (coarse — partfile has ~20 fields that change every tick during active download; per-second granularity matches reality)
  3. Loop change + backstopGet_EC_Response_GetUpdate snapshots s_globalEcGen, skips files whose m_ecGen ≤ the threshold from the last response, updates the threshold afterwards. Every 5 minutes a forced full sweep runs as a safety net for any missed hook — worst-case staleness if a hook was missed somewhere is bounded to that window rather than indefinite. 5 min is short enough to be invisible in the GUI and rare enough that the overhead is negligible (one full sweep per ~300 INC_UPDATE cycles at amulegui's ~1 Hz rate).

Backwards compatibility

Wire format unchanged. INC_UPDATE responses now contain only the files that changed since the last response, which is exactly what the delta protocol was designed to convey but the implementation used to emit empty per-file tag bodies for unchanged files anyway. Backwards-compatible with any amulegui / amuleweb / amulecmd version, old or new — the client side already correctly handles "no file tag = no change for this file".

Memory cost

  • 8 bytes per CKnownFile (atomic uint64) — ~700 KB on a 91 k-file shareset.
  • 8 + 8 bytes per CECServerSocket for the per-connection tracking — negligible.
  • One process-wide atomic uint64.

Test plan

  • macOS Debug build of amuled, amulecmd, amulegui — clean compile, no crash on basic startup / shutdown sanity.
  • Live amulegui (or amuleweb) connected to amuled with a large shareset — this is the actual path the PR optimises. Expectations:
  • perf record -p amuled -g -F 99 -- sleep 30 against a real workload to confirm the delta

Refs #713.

@got3nks

got3nks commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Update — two follow-up commits address a protocol-invariant regression and extend the optimization to amuleweb.

The bug (caught by @Stoatwblr in #713): amulegui's INC_UPDATE handler infers deletions from absence — any file ID missing from the response is removed from the local view. Skipping unchanged files entirely meant most of a 91k-file shareset disappeared every cycle, then the 5-min backstop re-added them, then they vanished again — wedging amulegui's list widget at 100% CPU on std::unordered_map::operator[] and wxListMainWindow::RebuildDataIndex churn. amulecmd was unaffected because it uses the FULL fetch path, not INC_UPDATE.

The fix — two new commits that keep the optimization in both paths:

  • 405e41f66 (4/5): backward-compat + partial-update protocol opt-in (amulegui)

    • Old amulegui (master) → server emits a 5-byte alive-marker tag (EC_TAG_KNOWNFILE / EC_TAG_PARTFILE with the ECID and no children) for unchanged files. The existing client gate if (tag->HasChildTags()) ProcessItemUpdate(...) already treats childless tags as a no-op update, so the bulk "missing == deleted" loop stays correct. The expensive per-file CEC_SharedFile_Tag construction + encoder pass is still skipped — that's the actual perf win. Only pays a tiny per-file wire cost.
    • New amulegui → advertises EC_TAG_CAN_PARTIAL_UPDATE at auth (parallel to EC_TAG_CAN_ZLIB etc.). When the server echoes it back in AUTH_OK, the server skips unchanged files entirely and emits explicit top-level EC_TAG_FILE_REMOVED markers for files that disappeared since the previous cycle. Client side skips its bulk deletion loop and processes only the explicit removals.
  • 0961d022f (5/5): extends the partial-update protocol to amuleweb

    • amuleweb uses EC_OP_GET_SHARED_FILES / EC_OP_GET_DLOAD_QUEUE with EC_DETAIL_UPDATE rather than the EC_OP_GET_UPDATE path amulegui uses. Same skip-unchanged + EC_TAG_FILE_REMOVED logic now wired into Get_EC_Response_GetSharedFiles and Get_EC_Response_GetDownloadQueue, guarded on the same opt-in flag and only kicking in for EC_DETAIL_UPDATE requests (so amulecmd's show shared is untouched). amuleweb's UpdatableItemsContainer::ProcessUpdate learns the same two-path behaviour.

Negotiation matrix:

  • new server + new client → full partial-update (skip + explicit removals)
  • new server + old client → alive-markers (no client change, ~all of the win)
  • old server + new client → server ignores capability, client falls back automatically
  • old server + old client → unchanged

Also drops the 5-minute paranoia backstop. With explicit removal markers and alive markers covering both protocol paths, the backstop's only remaining job was masking missed MarkECChanged() hooks — which actively makes those bugs harder to find. The protocol is now self-correcting.

Wire-format additions

Two new EC tag IDs in ECCodes.h / ECCodes.abstract:

EC_TAG_CAN_PARTIAL_UPDATE   0x0012   // auth-time capability + AUTH_OK echo
EC_TAG_FILE_REMOVED         0x0013   // top-level, value = ECID, one per removed file

Old daemons / clients ignore unknown tags, so both halves of the negotiation are forward-compatible.

Test plan update

  • Local smoke test (Mac): amulegui connects with Partial update: yes, INC_UPDATE responses settle at ~74 bytes per cycle in steady state, 0.2-0.6% CPU. Deletion of a shared file propagates correctly via explicit removal marker.
  • Local smoke test (Mac): amuleweb connects with Partial update: yes, dashboard renders both shared files, deletion shows up on the next page load.
  • Live amulegui + amuleweb on the 91k-file shareset — Stoatwblr is set up to retest against the refreshed combined/ec-perf-stack-713 branch.

Still draft pending real-workload soak.

@got3nks

got3nks commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

One more commit — dfac5b0a8 (6/6): closes nine MarkECChanged() gaps surfaced by a reverse audit (every EC_DETAIL_UPDATE field → mutation sites → coverage check).

Background: DownloadQueue::Process() only calls CPartFile::Process() for PS_READY/PS_EMPTY partfiles, so the per-tick mark we relied on in commit (3/5) doesn't fire when a partfile is paused, stopped, hashing, or completed. The 5-min backstop (dropped in commit 4/5) used to paper this over; without it, user actions like pause/resume/priority/category wouldn't propagate to amulegui or amuleweb until something else on the file changed.

Hooks added (all conditional on actual value change):

  • StopFile / PauseFile / ResumeFileEC_TAG_PARTFILE_STOPPED + _STATUS
  • SetDownPriority / SetAutoDownPriorityEC_TAG_PARTFILE_PRIO (auto flag folded in via +10 offset)
  • SetA4AFAutoEC_TAG_PARTFILE_A4AFAUTO
  • SetCategoryEC_TAG_PARTFILE_CAT
  • SetHashingProgressEC_TAG_PARTFILE_HASHED_PART_COUNT (hashing runs outside Process()'s gate too)
  • UpdateFileRatingCommentAvailEC_TAG_PARTFILE_COMMENTS (fires when sources deliver ratings/comments via OP_MESSAGE)

Refreshed combined/ec-perf-stack-713 (force-pushed with the cherry-pick of this commit on top).

@got3nks
got3nks marked this pull request as ready for review May 26, 2026 22:12
@mrjimenez

Copy link
Copy Markdown
Contributor

There is a conflict in KnownFile.cpp.

got3nks added 8 commits May 27, 2026 01:08
Adds the data + plumbing for skipping unchanged files in INC_UPDATE
responses (amule-project#713). No callers yet, no behavior change — pure
infrastructure to make the next two commits reviewable on their own.

Each CKnownFile carries an atomic uint64 generation counter. A process-
wide atomic global counter ensures every Mark…() yields a strictly-
ascending value across all files and all threads (CFileStatistic
counters tick from upload-disk-IO + hashing threads, not just main).

`MarkECChanged()` uses a single fetch_add(1) + store rather than a load /
increment / store so concurrent mutations from different threads can't
collide on the same gen.

Memory cost: 8 bytes per CKnownFile (~700 KB for a 91 k-file shareset).
One additional atomic uint64 at file scope.

Refs amule-project#713.
Wires `MarkECChanged()` into each storage primitive that writes a field
exported via `CEC_SharedFile_Tag` / `CEC_PartFile_Tag`. Still no behavior
change on the EC dispatch side — the next commit consumes the flag.

Sites covered:

  * `CFileStatistic::AddRequest / AddAccepted / AddTransferred`
    (EC_TAG_KNOWNFILE_REQ_COUNT, _ACCEPT_COUNT, _XFERRED and their _ALL
    counterparts). `AddTransferred` is the hottest of the three —
    every uploaded chunk flips the flag. The Mark op is a single
    atomic fetch_add+store so the cost is small even on multi-slot
    upload nodes.
  * `CKnownFile::AddUploadingClient / RemoveUploadingClient`
    (EC_TAG_KNOWNFILE_ON_QUEUE = m_ClientUploadList.size()).
  * `CKnownFile::SetFileName` (EC_TAG_PARTFILE_NAME and ed2k:// link).
  * `CKnownFile::SetFilePath` (EC_TAG_KNOWNFILE_FILENAME on non-partfiles).
  * `CKnownFile::SetUpPriority / SetAutoUpPriority` (EC_TAG_KNOWNFILE_PRIO,
    which folds the auto flag in as +10).
  * `CKnownFile::SetFileCommentRating` (EC_TAG_KNOWNFILE_COMMENT / _RATING).
  * `CKnownFile::UpdatePartsInfo` (EC_TAG_KNOWNFILE_COMPLETE_SOURCES{,LOW,HIGH}).
  * `m_pAICHHashSet->SetMasterHash` call sites in KnownFile.cpp /
    PartFile.cpp and `SetAICHHashset` swap in amule.cpp's hash-task
    completion (EC_TAG_KNOWNFILE_AICH_MASTERHASH).
  * `CPartFile::Process` — coarse hook at the per-second per-partfile
    tick. Partfiles have ~20 EC-exported fields that change
    independently on active downloads; the per-second tick is the
    right granularity and avoids hooking each of them.

A 60 s periodic backstop in commit (3/3) catches anything missed here.

Refs amule-project#713.
Consumes the per-file `m_ecGen` from commit (1/3) and the `MarkECChanged`
hooks from commit (2/3): `Get_EC_Response_GetUpdate` now skips files
whose generation is at or below the highest gen this connection has
already reflected in its `CObjTagMap`. For a 91 k-file shareset with a
few hundred actively-transferring files at any moment, the per-cycle
iteration goes from O(N) to O(files-that-actually-changed) — typically
a 99 %+ reduction.

Mechanism per connection:

  * snapshot = CKnownFile::s_globalEcGen at request start
  * threshold = m_lastEcGenSeen carried from the previous response
  * iterate only files where file.m_ecGen > threshold
  * m_lastEcGenSeen := snapshot after iteration
  * files whose m_ecGen changes mid-iteration land above the snapshot
    and get picked up on the next request

Backstop: every kFullEcSyncIntervalSeconds (60 s) every connection
runs an unconditional full sweep regardless of gen. Worst-case
staleness if a `MarkECChanged()` hook was missed somewhere is bounded
to that interval rather than indefinite. The interval is short enough
to keep amulegui's UI from drifting noticeably and long enough that
the 99 % skip win still dominates the per-cycle workload.

Wire format unchanged; INC_UPDATE responses now contain only the
files that changed since the last response, which is exactly what the
delta protocol was designed to convey but the implementation used to
emit empty tag bodies for unchanged files anyway. Backwards-compatible
with any client version (the client side already correctly handles
missing-file-tag = "no change").

Refs amule-project#713.
…t-in

Commit 3 broke amulegui on big libraries because amulegui infers
deletions from absence: any file ID missing from an INC_UPDATE response
is removed from the local view. Skipping unchanged files entirely meant
most of the shareset disappeared every cycle (then the 5-min backstop
re-added them), wedging the GUI's list-widget at 100% CPU in
std::unordered_map churn.

Two-pronged fix preserving the optimization in both compat and opt-in
paths:

* Backward compat (old amulegui / amuleweb / third-party client):
  unchanged files get a 5-byte "alive marker" tag — EC_TAG_KNOWNFILE /
  EC_TAG_PARTFILE with the ECID and no children. The legacy client
  already gates updates on `if (tag->HasChildTags())`, so empty tags
  count as "present" without triggering the per-item update. Keeps the
  expensive CEC_SharedFile_Tag construction + encoder pass off the
  hot path (the actual perf win); only pays a tiny per-file wire cost.

* Partial-update protocol opt-in:
  - New EC_TAG_CAN_PARTIAL_UPDATE capability advertised by the client
    at auth, mirrored back by the server in AUTH_OK if it understands
    the protocol.
  - When negotiated, server skips unchanged files entirely and emits
    explicit EC_TAG_FILE_REMOVED markers (top-level, value = ECID)
    for files that disappeared since the previous cycle.
    CECServerSocket tracks the previous cycle's snapshot in
    m_lastSentFileIds.
  - amulegui clients in this mode skip the bulk "missing == deleted"
    loop and process explicit removals only.

Also drops the 5-minute paranoia backstop. With explicit removal
markers and alive markers covering both protocol paths, the backstop's
only remaining job was hiding missed MarkECChanged() hooks — making
those bugs harder to find rather than easier.

Negotiation matrix:
  new server + new client → partial-update (full win, ~0 work
    per-cycle for static files)
  new server + old client → alive markers (almost full win, cheap
    per-file wire cost)
  old server + new client → server ignores capability, doesn't mirror
    it back; client falls back to bulk-deletion path
  old server + old client → unchanged
amuleweb drives its INC_UPDATE polling through `EC_OP_GET_SHARED_FILES`
+ `EC_OP_GET_DLOAD_QUEUE` with `EC_DETAIL_UPDATE` rather than the
`EC_OP_GET_UPDATE` path amulegui uses. Commit (4/5) wired the partial-
update protocol only into `Get_EC_Response_GetUpdate`, so amuleweb
kept iterating all files every cycle even on huge libraries.

Extend the same skip-unchanged + `EC_TAG_FILE_REMOVED` logic into
`Get_EC_Response_GetSharedFiles` and `Get_EC_Response_GetDownloadQueue`,
guarded on the same `m_partialUpdateActive` flag and only kicking in
for `EC_DETAIL_UPDATE` requests (so amulecmd's `show shared` and other
`EC_DETAIL_FULL` callers are untouched).

Per-path state because the three handlers fire on independent cadences:

* `m_lastEcGenSeen`        + `m_lastSentFileIds`        — amulegui's
  `EC_OP_GET_UPDATE`
* `m_lastEcGenSeenShared`  + `m_lastSentSharedFileIds`  — amuleweb's
  `EC_OP_GET_SHARED_FILES`
* `m_lastEcGenSeenPart`    + `m_lastSentPartFileIds`    — amuleweb's
  `EC_OP_GET_DLOAD_QUEUE`

amuleweb's `UpdatableItemsContainer::ProcessUpdate` learns the same
two-path behaviour: when the negotiated capability is active, drain
`EC_TAG_FILE_REMOVED` markers into a buffered removal set and run a
single sweep over the live list only when removals exist; otherwise
the legacy "missing == deleted" loop stays correct (server still emits
diff-empty alive-marker tags via the existing encoder path).

`CaMuleExternalConnector::IsServerPartialUpdateActive()` exposes
`CRemoteConnect::ServerSupportsPartialUpdate()` to the container.

Compat matrix unchanged from (4/5): the negotiation is symmetric, so
old amuleweb + new amuled and new amuleweb + old amuled both fall back
to the legacy path automatically.
…ction setters

Reverse audit of every EC_DETAIL_UPDATE field surfaced nine mutation
sites that change exported state without marking the partfile changed.
For an active partfile the per-tick `MarkECChanged()` at the top of
`CPartFile::Process()` covered them implicitly, but `DownloadQueue::
Process()` gates `Process()` to `PS_READY`/`PS_EMPTY` only — paused,
stopped, hashing, and completed partfiles never auto-mark, so a user
toggling pause/resume/priority/category from amulegui or amuleweb
would stay invisible until something else on the file changed.
The 5-minute paranoia backstop (removed in 4/5) used to paper over
this; explicit hooks are the correct fix.

Sites covered:

* `CPartFile::StopFile`              — EC_TAG_PARTFILE_STOPPED / _STATUS
* `CPartFile::PauseFile`             — EC_TAG_PARTFILE_STOPPED / _STATUS
* `CPartFile::ResumeFile`            — EC_TAG_PARTFILE_STOPPED / _STATUS
* `CPartFile::SetDownPriority`       — EC_TAG_PARTFILE_PRIO
* `CPartFile::SetAutoDownPriority`   — EC_TAG_PARTFILE_PRIO (auto flag
                                        folded in via +10 offset)
* `CPartFile::SetA4AFAuto`           — EC_TAG_PARTFILE_A4AFAUTO
* `CPartFile::SetCategory`           — EC_TAG_PARTFILE_CAT
* `CPartFile::SetHashingProgress`    — EC_TAG_PARTFILE_HASHED_PART_COUNT
                                        (runs during hashing, outside
                                        Process()'s gate)
* `CPartFile::UpdateFileRatingCommentAvail` — EC_TAG_PARTFILE_COMMENTS
                                        (fires when sources deliver
                                        ratings/comments via OP_MESSAGE)

All hooks are conditional on actual value change to keep the
generation counter from incrementing on no-op writes.
@got3nks

got3nks commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

There is a conflict in KnownFile.cpp.

Solved

@got3nks
got3nks force-pushed the feat/ec-skip-unchanged-files branch from dfac5b0 to 035304c Compare May 26, 2026 23:12
@mrjimenez
mrjimenez merged commit 2aa5f42 into amule-project:master May 26, 2026
7 checks passed
@got3nks
got3nks deleted the feat/ec-skip-unchanged-files branch May 27, 2026 15:15
got3nks added a commit to got3nks/amule that referenced this pull request May 27, 2026
…nownFile (amule-project#748)

CKnownFilesRem::DeleteItem was destroying a CKnownFile without
sweeping the client list to null out CUpDownClient::m_uploadingfile
/ m_reqfile on clients that still pointed at it. Sequence:

1. EC_TAG_FILE_REMOVED arrives for file X → CKnownFilesRem::DeleteItem(X)
   → X is destroyed.
2. Any CUpDownClient whose m_uploadingfile == X now holds a dangling
   pointer.
3. Some later INC_UPDATE batch removes that client →
   CUpDownClientListRem::DeleteItem(client) at line 1504 null-checks
   m_uploadingfile (non-null, passes the guard), line 1505 calls
   m_uploadingfile->RemoveUploadingClient(client) → operates on the
   freed CKnownFile, std::set::erase walks freed m_ClientUploadList
   nodes via CClientRef::operator< → SEGV.

Fix is a single-line callout from CKnownFilesRem::DeleteItem to a
new CUpDownClientListRem::DropReferencesTo(file) helper, which has
the friend access to CUpDownClient's private fields that
CKnownFilesRem lacks. O(N_clients) per file removal, bounded
because EC_TAG_FILE_REMOVED markers are rare (only fire when
amuled actually drops a known file, not on every INC_UPDATE).

The bug existed pre-amule-project#727 in principle but the legacy "anything
missing from the response is deleted" sweep tended to clean the
dangling-pointer clients out of the list before the next time
their DeleteItem ran. With explicit EC_TAG_FILE_REMOVED markers
(opt-in via EC_TAG_CAN_PARTIAL_UPDATE), file deletions are rare
and clients can hold the stale pointer arbitrarily long — heavy
sharesets like the one reporting hit the timing window readily.

Refs amule-project#748.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants