Rework OstreeAsyncProgress API to use GVariant #819
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OstreeAsyncProgress currently does some contortions to try and avoid allocating space for guints and guint64s (on 64-bit platforms), but this means it uses two GHashTables. A GHashTable allocates 8 buckets even when empty. Given that the largest usage of OstreeAsyncProgress in libostree puts 13 uints and 5 uint64s in it, this optimisation does not save significant (if any) memory. Instead, change OstreeAsyncProgress to store values internally as GVariants, and expose this with some new API: • ostree_async_progress_get_variant() • ostree_async_progress_set_variant() Each GVariant is allocated on the heap. As they are immutable, they are thread-safe once returned by a getter. The existing API continues to work as before, except in the case where a key is set/got as both a uint and a uint64 — there will now be a collision (and a GVariant type checking failure) whereas previously there was no collision. Nothing in OSTree uses OstreeAsyncProgress this way though. The new API can be used to share more complex data via the progress API. Signed-off-by: Philip Withnall <withnall@endlessm.com>
OstreeAsyncProgress is thread-safe: it can have keys changed by one thread while another is getting the same keys (modulo some locking contention). However, the thread safety is done at the function call level: if some code calls an OstreeAsyncProgress getter several times, the key fetches are not atomic with respect to each other. In the case of contention on the lock, this can result in consumers of OstreeAsyncProgress data seeing an inconsistent state between the properties they query, which could result in progress reporting inaccuracies. In the uncontested case, this results in the OstreeAsyncProgress lock being locked and unlocked many times more than necessary. Try to improve this by adding new API, which supports getting and setting multiple keys atomically: • ostree_async_progress_get() • ostree_async_progress_set() The new API uses GVariants and varargs: keys are passed as a GVariantType string followed by arguments as for g_variant_new() or g_variant_get(), followed by the next key, etc. Signed-off-by: Philip Withnall <withnall@endlessm.com>
This will eliminate most of the potential races in progress reporting. ostree_repo_pull_default_console_progress_changed() still calls three getters, so there may still be races there, however. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Rework how the status is handled in OstreeAsyncProgress so that it’s now a well-known key in the hash table. This means that it can be retrieved and set atomically with other keys using ostree_async_progress_[get|set](). The behaviour of ostree_async_progress_[get|set]_status() is preserved, with the caveat that `status` can now also be accessed using the other API on OstreeAsyncProgress, and has to be accessed with the right GVariant type. Internally, a NULL status is represented by an empty status string (since ostree_async_progress_[get|set]_variant() deliberately don’t allow NULL variants to be set against keys, since that would break the ostree_async_progress_get() API). Signed-off-by: Philip Withnall <withnall@endlessm.com>
Use the new well-known `status` key for OstreeAsyncProgress to get and set the status atomically with other keys in an OstreeAsyncProgress instance. Signed-off-by: Philip Withnall <withnall@endlessm.com>
|
The flatpak failure isn't your fault, I submitted a PR to fix it. The travis one though looks real: I think we just need an Otherwise: Awesome! A++, would review again! |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 29, 2017
OstreeAsyncProgress is thread-safe: it can have keys changed by one thread while another is getting the same keys (modulo some locking contention). However, the thread safety is done at the function call level: if some code calls an OstreeAsyncProgress getter several times, the key fetches are not atomic with respect to each other. In the case of contention on the lock, this can result in consumers of OstreeAsyncProgress data seeing an inconsistent state between the properties they query, which could result in progress reporting inaccuracies. In the uncontested case, this results in the OstreeAsyncProgress lock being locked and unlocked many times more than necessary. Try to improve this by adding new API, which supports getting and setting multiple keys atomically: • ostree_async_progress_get() • ostree_async_progress_set() The new API uses GVariants and varargs: keys are passed as a GVariantType string followed by arguments as for g_variant_new() or g_variant_get(), followed by the next key, etc. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #819 Approved by: cgwalters
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 29, 2017
This will eliminate most of the potential races in progress reporting. ostree_repo_pull_default_console_progress_changed() still calls three getters, so there may still be races there, however. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #819 Approved by: cgwalters
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 29, 2017
Rework how the status is handled in OstreeAsyncProgress so that it’s now a well-known key in the hash table. This means that it can be retrieved and set atomically with other keys using ostree_async_progress_[get|set](). The behaviour of ostree_async_progress_[get|set]_status() is preserved, with the caveat that `status` can now also be accessed using the other API on OstreeAsyncProgress, and has to be accessed with the right GVariant type. Internally, a NULL status is represented by an empty status string (since ostree_async_progress_[get|set]_variant() deliberately don’t allow NULL variants to be set against keys, since that would break the ostree_async_progress_get() API). Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #819 Approved by: cgwalters
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 29, 2017
Use the new well-known `status` key for OstreeAsyncProgress to get and set the status atomically with other keys in an OstreeAsyncProgress instance. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #819 Approved by: cgwalters
|
☀️ Test successful - status-atomicjenkins |
|
I'm hitting an assertion with this: This seems to happen from |
pwithnall
added a commit
to pwithnall/ostree
that referenced
this pull request
May 5, 2017
If one of the progress keys is set in a pull operation, a ::changed signal is emitted on the progress object, and the callback for that could query any of the progress keys — so they all need to be set, otherwise we get an assertion failure in ostree_async_progress_get() due to a named key not existing. Spotted by Dan Nicholson in PR ostreedev#819. Signed-off-by: Philip Withnall <withnall@endlessm.com>
|
I can’t reproduce the exact crash you’re seeing, but I think I can see where the problem is. ⇒ #835. |
rh-atomic-bot
pushed a commit
that referenced
this pull request
May 5, 2017
If one of the progress keys is set in a pull operation, a ::changed signal is emitted on the progress object, and the callback for that could query any of the progress keys — so they all need to be set, otherwise we get an assertion failure in ostree_async_progress_get() due to a named key not existing. Spotted by Dan Nicholson in PR #819. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #835 Approved by: cgwalters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I spent a little while today looking at different progress reporting APIs, and it turns out that despite @cgwalters’ reservations,
OstreeAsyncProgressis pretty much ideal. I’ve pasted my analysis at the bottom of this PR.This PR adds a few missing features to
OstreeAsyncProgresswhich will allow it to be used by the LAN/USB work (PR #782) to give structured information about each ref it’s pulling, for example, by passingGVariantdicts in the progress information.The PR also improves the atomicity of accesses to an
OstreeAsyncProgressinstance, which fixes some potential bugs in reporting inconsistent pull progress.Options for progress reporting:
EvPrintOperation: have an object for the entire operation, report progress via its properties or methodsFlatpakProgressCallback,GFileProgressCallback: caller provides a callback which has a fixed set of parametersOstreeAsyncProgress: caller provides an object which has a notification callback associated with it