chore: LockTable tracks fingerprints of keys #2839

romange · 2024-04-04T07:15:35Z

It's a first step that will probably simplify dependencies in many places where we need to keep key strings for that. A second step will be to reduce the CPU load of multi-key operations like MSET and precompute Fingerprints once.

chakaz · 2024-04-04T07:26:16Z

src/server/table.h

+  // We use fingerprinting before accessing locks - no need to mix more.
+  struct Hasher {
+    size_t operator()(uint64_t val) const {
+      return val;
    }
-
-    // If the key is backed by a string_view, replace it with a string with the same value
-    void MakeOwned() const;
-
-    mutable std::variant<std::string_view, std::string> val_;
  };
-
-  absl::flat_hash_map<Key, IntentLock> locks_;
+  absl::flat_hash_map<uint64_t, IntentLock, Hasher> locks_;


Why do we need a custom hasher instead of just absl::flat_hash_map<uint64_t, IntentLock>?

line 104 explains. If it's not clear I will expand there.

src/server/table.h

src/server/transaction.h

dranikpg

Please share benchmark results. I like that we remove code, but I doubt it's faster, we just moved the hashing part to our side

src/server/table.h

romange · 2024-04-04T09:02:33Z

Please share benchmark results. I like that we remove code, but I doubt it's faster, we just moved the hashing part to our side

Exactly right. Because it's a half-work. Doing the second half :)

romange · 2024-04-04T10:50:36Z

Ok, I implemented Release/Acquire with fingerprints. the performance difference is negligible for MSET.
@dranikpg I am fine to sack this PR but I think it will simplify things for multi transactions as well.
See the second half here: https://github.com/dragonflydb/dragonfly/compare/Pr6?expand=1

dranikpg

Okay, so given it doesn't have much of performance advantages

I'm not opposed to the idea if it simplifies code. However, in many cases we need to now keep track of both string and fingerprints, not sure if it's a simplification on average over the long run.
I still don't understand why you insist on making CheckLock a member function on transaction. It never belonged there, especially as a member. We spent so much effort previously to remove all excessive code from it 😿

romange · 2024-04-05T08:47:56Z

(2) is not important. And I do not insist on this. it's just is currently used only there but we can move it back to DbSlice.
I agree that we need to keep both fps and keys because we need to know which keys belong to which shards.

dranikpg

You can move (2) to anonymous namespace. I'm being annoyingly picky because of https://en.wikipedia.org/wiki/Broken_windows_theory. First glass shattering 😆

(1) Is up to you. I don't have strong opinions as long as it's maintainable

romange · 2024-04-10T06:53:30Z

I decided to proceed with this PR. I believe this PR will help me to implement more advanced heuristics of VLL paper, where we can identify OOO Transactions in the TX queue even before they reach the TX-queue head. Not something we must do urgently but I think it does simplify things with everything related to key locking.

romange · 2024-04-10T08:13:29Z

src/server/transaction.cc

-                 ", contended locks: ", info.contended_locks, "\n");
+      string msg = StrCat("TxQueue is too long. Tx count:", info.tx_total,
+                          ", armed:", info.tx_armed, ", runnable:", info.tx_runnable,
+                          ", global:", info.tx_global, ", total locks: ", info.total_locks,


just added global stats

It's a first step that will probably simplify dependencies in many places where we need to keep key strings for that. A second step will be to reduce the CPU load of multi-key operations like MSET and precompute Fingerprints once. Signed-off-by: Roman Gershman <roman@dragonflydb.io>

chakaz · 2024-04-10T09:54:23Z

src/server/table.h

  bool Acquire(std::string_view key, IntentLock::Mode mode);
  void Release(std::string_view key, IntentLock::Mode mode);


Here's a proposal: have overloads for both of these functions, the existing one (accepting a string_view), and another one accepting a fingerprint.
Then have the string_view version do the hash and call the overloaded one. This will save you the trouble of adding them in the future PR you plan to add.

I already wrote this in another branch. I prefer adding them in the next PR but I did like you suggested.

chakaz · 2024-04-10T09:54:46Z

src/server/table.h

@@ -92,17 +89,24 @@ class LockTable {
  bool Acquire(std::string_view key, IntentLock::Mode mode);
  void Release(std::string_view key, IntentLock::Mode mode);

+  static uint64_t Fingerprint(std::string_view key);


Consider some typedef for the fingerprint type, like using THash = uint64_t

It won't grow, it won't shrink, keeps less types. std::hash uses size_t as well after all

giving a name to this "type" makes the code easier to read (like that it's not an index but a hash). once C++ will get distinct typedef it'll be even better, because then the compiler can guard against such errors

…nfly ( v1.16.1 → v1.17.0 ) (#3473) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker.dragonflydb.io/dragonflydb/dragonfly](https://togithub.com/dragonflydb/dragonfly) | minor | `v1.16.1` -> `v1.17.0` | --- ### Release Notes <details> <summary>dragonflydb/dragonfly (docker.dragonflydb.io/dragonflydb/dragonfly)</summary> ### [`v1.17.0`](https://togithub.com/dragonflydb/dragonfly/releases/tag/v1.17.0) [Compare Source](https://togithub.com/dragonflydb/dragonfly/compare/v1.16.1...v1.17.0) ##### Dragonfly v1.17.0 Some prominent changes include: - Improved performance for MGET operations ([#2453](https://togithub.com/dragonflydb/dragonfly/issues/2453)) - Fix argument parsing in json.objkeys ([#2872](https://togithub.com/dragonflydb/dragonfly/issues/2872)) - Fix ipv6 support for replication ([#2889](https://togithub.com/dragonflydb/dragonfly/issues/2889)) - Support serialisation of bloom filters - saving to and loading from snapshots ([#2846](https://togithub.com/dragonflydb/dragonfly/issues/2846)) - Support of HLL PFADD ([#2761](https://togithub.com/dragonflydb/dragonfly/issues/2761)) - Support bullmq workloads that do not have `{}` hashtags in their queue names ([#2890](https://togithub.com/dragonflydb/dragonfly/issues/2890)) ##### What's Changed - fix: [#2745](https://togithub.com/dragonflydb/dragonfly/issues/2745) don't start migration process again after apply the same the same config is applied by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2822 - feat(transaction): Idempotent callbacks (immediate runs) by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2453 - refactor(cluster): replace sync_id with node_id for slot migration [#2835](https://togithub.com/dragonflydb/dragonfly/issues/2835) by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2838 - feat(tiering): Simple OpManager by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2781 - chore: implement path mutation for JsonFlat by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2805 - feat(cluster): add migration removing by config [#2835](https://togithub.com/dragonflydb/dragonfly/issues/2835) by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2844 - chore: expose direct API on Bloom objects by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2845 - chore: generalize CompactObject::AllocateMR by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2847 - feat(tiering): Simplest small bins by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2810 - refactor: clean cluster slot migration code by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2848 - fix(tests): Fix numsub test by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2852 - fix: healthcheck for docker containers by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2853 - fix: possible crash in tls code by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2854 - fix(server): Do not block admin-port commands by [@chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2842 - fix(pytest): make pytests fail if server crash on shutdown by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2827 - feat(server): add prints on takeover timeout by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2856 - fix(pytest): dont check process return code on kill by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2862 - fix: authorize the http connection to call commands by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2863 - feat(cluster): Send number of keys for incoming and outgoing migrations. by [@chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2858 - feat(tiering): TieredStorageV2 by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2849 - bug(server): set connection flags block/pause flag on all blocking commands by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2816 - chore: serialize SBF by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2846 - fix: test_replicaof_reject_on_load crash on stop by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2818 - feat(dbslice): Add self-laundering iterator in `DbSlice` by [@chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2815 - chore: License update by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2767 - fix(acl): incompatibilities with acl load by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2867 - fix(json): make path optional in json.objkeys by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2872 - fix: return wrong type errors for SET...GET command by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2874 - fix(redis replication): remove partial sync flow ,not supported yet by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2865 - chore: limit traffic logger only to the main interface by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2877 - chore: relax repltakeover constraints to only exclude write commands by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2873 - chore(replayer): Roll back to go1.18 by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2881 - fix: brpoplpush single shard to wake up blocked transactions by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2875 - chore: LockTable tracks fingerprints of keys by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2839 - chore: reject TLS handshake when our listener is plain TCP by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2882 - Add support for Sparse HLL PFADD by [@azuredream](https://togithub.com/azuredream) in [dragonflydb/dragonfly#2761 - feat server: bring visibility to script errors by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2879 - chore: clean up REPLTAKEOVER flow by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2887 - chore(tiering): Move files and move kb literal to common by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2868 - chore(interpreter): Support object replies by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2885 - fix(ci/helm): Stick to v0.73.0 version of prom operator by [@Pothulapati](https://togithub.com/Pothulapati) in [dragonflydb/dragonfly#2893 - fix(acl): authentication with UDS socket by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2895 - feat(cluster): add repeated ACK if an error is happened by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2892 - chore(blocking): Remove faulty DCHECK by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2898 - chore: add a clear link on how to build dragonfly from source by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2884 - feat(server): Allow configuration of hashtag extraction by [@chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2890 - fix: fix build under macos by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2901 - fix(cluster_replication): replicate redis cluster node bug fix by [@adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2876 - fix(acl): skip http and add check on connection traversals by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2883 - fix(zset): Better memory consumption calculation by [@chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2900 - fix: fix ld for num converting by [@BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2902 - chore: add help string for memory_fiberstack_vms_bytes by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2903 - fix(sanitizers): false positive fail on multi_test::Eval by [@kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2896 - chore: pull helio and add ipv6 replication test by [@dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2889 - chore: add ipv6 support for native linux release by [@romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2908 ##### Huge thanks to all the contributors! ❤️ **Full Changelog**: dragonflydb/dragonfly@v1.16.0...v1.17.0 </details>  Co-authored-by: repo-jeeves[bot] <106431701+repo-jeeves[bot]@users.noreply.github.com>

romange requested a review from chakaz April 4, 2024 07:15

romange force-pushed the Pr5 branch from 9c77eba to 6f92292 Compare April 4, 2024 07:26

chakaz reviewed Apr 4, 2024

View reviewed changes

romange force-pushed the Pr5 branch from 6f92292 to 66488a7 Compare April 4, 2024 07:43

dranikpg reviewed Apr 4, 2024

View reviewed changes

src/server/transaction.h Outdated Show resolved Hide resolved

dranikpg reviewed Apr 4, 2024

View reviewed changes

src/server/table.h Outdated Show resolved Hide resolved

romange requested a review from dranikpg April 4, 2024 14:02

dranikpg reviewed Apr 5, 2024

View reviewed changes

dranikpg previously approved these changes Apr 5, 2024

View reviewed changes

romange dismissed dranikpg’s stale review via c0e2015 April 10, 2024 06:07

romange force-pushed the Pr5 branch 3 times, most recently from 61b4eea to 40db258 Compare April 10, 2024 06:31

romange requested review from chakaz and dranikpg April 10, 2024 08:09

romange commented Apr 10, 2024

View reviewed changes

romange force-pushed the Pr5 branch from 40db258 to 273e516 Compare April 10, 2024 09:32

romange force-pushed the Pr5 branch from 273e516 to b2d1a47 Compare April 10, 2024 09:43

chakaz previously approved these changes Apr 10, 2024

View reviewed changes

chore: comments

ddb486a

romange dismissed chakaz’s stale review via ddb486a April 10, 2024 11:40

chakaz previously approved these changes Apr 10, 2024

View reviewed changes

chore: pull helio for tls fixes

d1c8ce3

romange dismissed chakaz’s stale review via d1c8ce3 April 10, 2024 12:12

romange requested a review from chakaz April 10, 2024 13:02

chakaz approved these changes Apr 10, 2024

View reviewed changes

romange merged commit da5c51d into main Apr 10, 2024
10 checks passed

romange deleted the Pr5 branch April 10, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: LockTable tracks fingerprints of keys #2839

chore: LockTable tracks fingerprints of keys #2839

romange commented Apr 4, 2024

chakaz Apr 4, 2024

romange Apr 4, 2024

dranikpg left a comment

romange commented Apr 4, 2024

romange commented Apr 4, 2024

dranikpg left a comment

romange commented Apr 5, 2024

dranikpg left a comment

romange commented Apr 10, 2024 •

edited

romange Apr 10, 2024

chakaz Apr 10, 2024

romange Apr 10, 2024

chakaz Apr 10, 2024

dranikpg Apr 10, 2024 •

edited

chakaz Apr 10, 2024

		bool Acquire(std::string_view key, IntentLock::Mode mode);
		void Release(std::string_view key, IntentLock::Mode mode);

chore: LockTable tracks fingerprints of keys #2839

chore: LockTable tracks fingerprints of keys #2839

Conversation

romange commented Apr 4, 2024

chakaz Apr 4, 2024

Choose a reason for hiding this comment

romange Apr 4, 2024

Choose a reason for hiding this comment

dranikpg left a comment

Choose a reason for hiding this comment

romange commented Apr 4, 2024

romange commented Apr 4, 2024

dranikpg left a comment

Choose a reason for hiding this comment

romange commented Apr 5, 2024

dranikpg left a comment

Choose a reason for hiding this comment

romange commented Apr 10, 2024 • edited

romange Apr 10, 2024

Choose a reason for hiding this comment

chakaz Apr 10, 2024

Choose a reason for hiding this comment

romange Apr 10, 2024

Choose a reason for hiding this comment

chakaz Apr 10, 2024

Choose a reason for hiding this comment

dranikpg Apr 10, 2024 • edited

Choose a reason for hiding this comment

chakaz Apr 10, 2024

Choose a reason for hiding this comment

romange commented Apr 10, 2024 •

edited

dranikpg Apr 10, 2024 •

edited