Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handling of Future handles #54

Closed
wants to merge 172 commits into from
Closed

Better handling of Future handles #54

wants to merge 172 commits into from

Conversation

KrzysFR
Copy link
Member

@KrzysFR KrzysFR commented Feb 16, 2015

This is tracking PR for the work of rewriting the way FDBFuture* handles are wrapped to .NET's Task<T>, as detailed in #48.

This PR changes the following:

  • FdbFutureHandle has been removed, and Futures handles are now back to IntPtr. This helps avoid problems when the GC wants to destroy handles at unwanted times.
  • Added the notion of "Future Context" which is used by all object that can create futures (currently only clusters and transactions). Each future context tracks all the futures that it created.
  • There is now a centralized static dictionary that holds all the contexts, and is used by the single callback delegate.
  • The 64-bit IntPtr cookie that is passed to the callback is split into two parts: the upper 32 bits encode the id of the context in the global dictionary. The lower 32 bits encode the id of the future in that context.
  • All futures implement the non-generic IFdbFuture, so that they can be put in the same dictionary or list.

…s [BROKEN]

- Future handles are no longer stored in safe handles, to prevent the GC from destroying the futures behind our back
- FdbFutureContext is the base class of all "contexts" that can have futures (ie: clusters, transactions, ...)
- There is a static dictionary that stores all active contexts (first routing step)
- Each context has also its own dictionary that stores all active futures in this context (second rounting step)
- future callback parameter is the concatenation of the context ID and the future ID and is used for the two-step routing
- Multi-futures (FdbFutureArray<T>) use refcounting and only fire once the last handle has fired
…llbacks

- maybe remove this when we are done?
…hread, and defer the handling to the thread pool

- When callback are invoked inline, we have to return ASAP to the caller (who holds a lock) so we move to the TP
- If we have already been moved to the TP, then we can call OnReady inline (already on the TP!) else we defer to the TP at that time.
- Use the 32 lower bits as the key in the future dictionary, because IntPtr is not IEquatable<IntPtr> and uses a boxing key comparer
- Added a "dead" flag to future context,  and implemented refcounting down to 0 to remove the context from the global MapToException
- Changed the way transaction are cancelled, by waiting for FDB_C to call us back for each pending future (need to map `transaction_cancelled` into a TaskCancelledException for this to work)
- Started working on a Watch refactoring
…ost of copying the execution context for nothing

- Used internally to trigger cancellation, and we do not care about the ExecutionContext to do that.
@KrzysFR KrzysFR added api Issues or changes related to the client API native Interop with the native client library labels Feb 16, 2015
@KrzysFR KrzysFR added this to the 0.9.9 milestone Feb 16, 2015
@KrzysFR KrzysFR self-assigned this Feb 16, 2015
…d transaction

- Cancellation will be handled by tr.Cancel() internally
@KrzysFR
Copy link
Member Author

KrzysFR commented Feb 16, 2015

The way futures are cancelled has changed. Before, each transaction would pass its own cancellation token to the future, which would register a callback to call fdb_future_cancel() which would make the operation stop. This meant a lot of calls to CancellationToken.Register(....) which is notoriously slow (capturing Execution Contexts, mutating sparse arrays in the parent CancellationTokenSource) and dangerous (adds a new layer of concurrency because now we can end up cancelling a future while its callback is running, or its handle being destroyed).

The new systems drops the mandatory cancellation registration, and relies on the fact that cancelling the transaction will make the future fail almost immediately with a transaction_cancelled error code, which would be remapped to a TaskCancelledException. This introduce an additional latency compared to before (the network thread has to run the callback for the task to cancel), but help solve the concurrency issue between cancellation and dispose (say when a unit test fails, and the whole AppDomain gets torn down).

The only kind of futures that must support individual cancellation, and would still require a CancellationTokenRegistration would be Watches, and could be handled differently.

- Using a profiler, and looking at the x64 assembly generated by the JIT, it seems that always calling memcmp / memmove is fast enough
- Added #define MEASURE to count and measure the invocation of memory copy and compares to get a feel of the most used sizes by typical applications
- Count 0, 1 and 2, which are most impacted by calling memcmp or memcpy, are less frequent. Most frequent in a reference app where size 5 (corresponds to a 4 bytes integer + the type prefix in tuple encoding) or 17 bytes (GUIDs in tuples).
- Reduced the number of method calls needed to invoke memcmp and memmove
- Made EnsureSliceIsValid and EnsureBufferIsValid inlined by default, to remove one more method call
- byte* ptr =&buffer[offset] : does a bound check which can fail if the array is empty.
- byte* ptr; ptr + offset : does not bound check
…t possible for byte copy and compare:

- The x64 assembly generated to get the addess of &left[offset] is smaller and faster, than getting the addess of left and adding the offset manually
- &left[offset] also does a nullcheck and boundcheck at runtime "for free"
…when possible

- Encoding.UTF8 returns a new object everytime it is called, so cache one and reuse it
- Use new string(sbyte*, ..., [Encoding]) instead of [Encoding].GetString(byte[], .....) whenever possible
KrzysFR added a commit that referenced this pull request Mar 6, 2015
@KrzysFR
Copy link
Member Author

KrzysFR commented Apr 1, 2015

Given the fact that the native API is no longer available, this PR doesn't have any reason for being, especially since the Watch API was not done yet.

I'll maybe extract a few tweaks and optimizations from this PR and move them to the master branch in another PR.

- Use Guid.NewGuid() to generate random bits
…, AsTyped<T..>(..) extension methods

- Version of Using(..) and UsingEncoder(...) that took an IKeyEncoding are renamed to AsDynamic() or AsTyped<T...>()
- The encoding is optional, and will use TuPack.Encoding by default
- Changed the KeySubspace.Copy(...) into extension methods
…ice, Slice) tuple internally

- Add implicit cast between KeyRange and (Slice, Slice) to make this invisible to users
… ITypeSystem to tie everything together

- IValueEncoding add methods to get value encoders
- ITypeSystem combines an IKeyEncoding and IValueEncoding
- Rename all IKeyEncoding.GetEncoder() to GetKeyEncoder() to not conflict with value encoders
- Refactor all sample layers to be more modern in the way they are configured.
abdullin added a commit to bitgn/foundationdb-dotnet-client that referenced this pull request Apr 28, 2018
Although FreeLibrary and dlclose aren't used right now, they will be
needed later after Doxense#54 is resolved
abdullin and others added 13 commits April 28, 2018 10:49
Although FreeLibrary and dlclose aren't used right now, they will be
needed later after #54 is resolved
We keep existing behavior as is for the windows platform and simplify
non-windows loading. On non-win we load the native library only if
explicitly provided
loading: on non-windows use libdl to load libfdb_c
- remove .nuget from the package (it was an old v2 that was not able to install packages anymore)
- update build.bat to download latest version of nuget and install FAKE4 and NUnit3 runners
- Use NUnit3 target for tests
- fix path to XML comment file
- string format as changed from "lower_case_keyword" to english text
- GetStorageEngineModeAsync(..) now recognize "ssd-2" storage engine
- note: we still return "ssd" for "ssd-1", not sure what it would break?
# Conflicts:
#	FoundationDB.Client/Core/IFdbTransactionHandler.cs
#	FoundationDB.Client/Fdb.cs
#	FoundationDB.Client/FdbTransaction.Snapshot.cs
#	FoundationDB.Client/FdbTransaction.cs
#	FoundationDB.Client/FdbWatch.cs
#	FoundationDB.Client/FoundationDB.Client.csproj
#	FoundationDB.Client/Native/FdbFuture.cs
#	FoundationDB.Client/Native/FdbFutureArray.cs
#	FoundationDB.Client/Native/FdbFutureSingle.cs
#	FoundationDB.Client/Native/FdbNative.cs
#	FoundationDB.Client/Native/FdbNativeCluster.cs
#	FoundationDB.Client/Native/FdbNativeTransaction.cs
#	FoundationDB.Client/Subspaces/Fdb.Directory.cs
#	FoundationDB.Client/Tuples/Encoding/TupleParser.cs
#	FoundationDB.Client/Utils/Slice.cs
#	FoundationDB.Client/Utils/SliceComparer.cs
#	FoundationDB.Client/Utils/SliceHelpers.cs
#	FoundationDB.Tests/Layers/TupleFacts.cs
#	FoundationDB.Tests/TransactionFacts.cs
#	FoundationDB.Tests/Utils/SliceFacts.cs
@KrzysFR
Copy link
Member Author

KrzysFR commented Apr 28, 2018

I merged master into that branch, and it is a lot of changes since then.

The code seems to work, but there are a lot of TODOs still in the code (most notably Watches are broken!) and "this should never happen".

Also, this PR is based from an old branch, and should probably recreated on top of master

@KrzysFR
Copy link
Member Author

KrzysFR commented Apr 28, 2018

Closing this and moving to #81 that is based off master

@KrzysFR KrzysFR closed this Apr 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues or changes related to the client API native Interop with the native client library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants