Command argument parsing improvements #255

PaulusParssinen · 2024-04-07T19:53:56Z

Culture invariant parsing

Avoid re-encoding ASCII and allocating temporary strings, instead feed bytes into directly to Utf8Parser.TryParse
- In 02cca16, this change indirectly fixes the issue where cultures with decimal seperator being comma instead of dot caused some of Geo RESP tests fail.
- UPDATE: Fixed all remaining floating point formatting too by passing InvariantCulture to Parse and ToString.
Avoid some CurrentCulture lookups by using ToUpperInvariant instead of ToUpper
Use StringComparison.OrdinalIgnoreCase where we otherwise allocated uppercase string for string comparison. This equality comparison is also much faster as it doesn't care about cultures.
- And use StringComparison.Ordinal where applicable for perf (it gets unrolled for constant inputs à la SequenceEquals).StringComparison.OrdinalIgnoreCase also gets unrolled for constants in .NET 8 but has to deal with some bit flipping for the case insensitivity.

Enums

Use Enum.TryParse instead of catching exceptions for Enum.Parse, much more cheaper.
Use generic enum parsing overloads, allows JIT to see the type and optimize it better.

Misc.

Removed unused methods from RespReadUtils
Added internal ArgSlice.FromPinnedSpan
- Necessary evil that will allow further simplifications in future contributions.
- In this PR, it allow simplifying the HashObject's IncrementIntegerOrFloat logic.

* We can avoid allocating intermediary arrays and extra fixing now

…Header

* Also few other nits for speed

…piate * Avoid string allocation & avoid current thread culture lookup for equals * Switched some ToUpper to ToUpperInvariant too

* Avoid CurrentCulture lookup

* Also switch over to generic overloads, allows JIT to see what type is used

* We completely spanify these later, the calls got ugly

Missed one.

* Thanks github web editor

libs/cluster/Session/ClusterCommands.cs

libs/cluster/Session/FailoverCommand.cs

badrishc · 2024-04-16T00:27:29Z

@vazois @lmaas - any additional comments on this PR? Is it ready for merge?

libs/server/Metrics/Info/InfoCommand.cs

libs/server/ACL/ACLParser.cs

libs/server/Resp/Bitmap/BitmapCommands.cs

* StringComparison.Ordinal gets unrolled by JIT

lmaas

Excellent work! There are a lot of good changes in this PR. Thank you for working on this.

I have added a few comments with suggestions for improvement. Let’s address those points before merging. Once we’ve resolved the questions, I’ll be happy to perform another review.

libs/server/ACL/ACLParser.cs

libs/server/Objects/Hash/HashObjectImpl.cs

libs/server/Objects/ObjectUtils.cs

libs/server/ACL/ACLParser.cs

* Comparisons strings to uppercase * Formatting adjustments * Few unnecessary using directives in the files which PR touches

libs/cluster/Server/ClusterConfig.cs

* Makes it clear that the methods don't do extra "work"

lmaas

Great job on the code! Your thorough work is much appreciated. I believe we’re ready to merge this PR. Thank you for your valuable contribution! 🙌

PaulusParssinen added 14 commits April 7, 2024 22:05

Add internal ArgSlice.FromPinnedSpan

ebb5521

Simplify paths where WriteIntegerFromBytes is used

1f9d5e2

* We can avoid allocating intermediary arrays and extra fixing now

Avoid ASCII re-encoding and string allocation in ReadDoubleWithLength…

02cca16

…Header

Remove unused ReadPtrArrayWithLengthHeader

b519827

Remove unused ReadArrayWithLengthHeader

2e0211b

Avoid ASCII re-encoding and string allocation in more places

7ec7da3

* Also few other nits for speed

Use StringComparison.OrdinalIgnoreCase instead of ToUpper where appro…

d729cdf

…piate * Avoid string allocation & avoid current thread culture lookup for equals * Switched some ToUpper to ToUpperInvariant too

ToLower -> ToLowerInvariant

25da8a4

* Avoid CurrentCulture lookup

Use Enum.TryParse instead of paying the cost exception

fa35433

* Also switch over to generic overloads, allows JIT to see what type is used

dotnet-format

e62d4c8

Revert BitmapCommand Parse overload changes

ad142b9

* We completely spanify these later, the calls got ugly

Update BitmapCommands.cs

b729f9a

Missed one.

Merge branch 'main' into avoid-ascii-re-encoding

2d5e12c

dotnet-format

281ef62

* Thanks github web editor

PaulusParssinen commented Apr 8, 2024

View reviewed changes

libs/cluster/Session/ClusterCommands.cs Outdated Show resolved Hide resolved

Update libs/cluster/Session/ClusterCommands.cs

c9ba659

lmaas added the parser label Apr 8, 2024

lmaas self-assigned this Apr 8, 2024

badrishc and others added 2 commits April 8, 2024 13:06

Merge branch 'main' into avoid-ascii-re-encoding

fb65861

FromFixedSpan -> FromPinnedSpan

1b42f8a

badrishc requested a review from vazois April 8, 2024 20:31

PaulusParssinen added 3 commits April 9, 2024 01:11

Merge branch 'main' into avoid-ascii-re-encoding

e81d435

Merge branch 'main' into avoid-ascii-re-encoding

178e0d4

Add import lost in merge

9fbb6b1

vazois requested changes Apr 9, 2024

View reviewed changes

libs/cluster/Session/ClusterCommands.cs Outdated Show resolved Hide resolved

libs/cluster/Session/FailoverCommand.cs Outdated Show resolved Hide resolved

PaulusParssinen added 2 commits April 9, 2024 19:48

Apply suggestions from code review

5ffe694

Merge branch 'main' into avoid-ascii-re-encoding

e0e80df

PaulusParssinen marked this pull request as draft April 9, 2024 19:59

PaulusParssinen added 2 commits April 10, 2024 00:27

Remove regex from ACLParser

0c04431

Use string.Equals in ACLParser

49acb3e

Fix double.TryParse overload for .NET 6

98f9050

PaulusParssinen changed the title ~~Minor command argument parsing improvements~~ Command argument parsing improvements Apr 10, 2024

PaulusParssinen requested a review from vazois April 11, 2024 17:53

badrishc requested a review from lmaas April 16, 2024 00:19

Merge branch 'main' into avoid-ascii-re-encoding

cef1f44

vazois approved these changes Apr 16, 2024

View reviewed changes

libs/server/Metrics/Info/InfoCommand.cs Outdated Show resolved Hide resolved

PaulusParssinen added 3 commits April 16, 2024 22:54

Merge branch 'main' into avoid-ascii-re-encoding

a6d8563

Use Enum.TryParse instead of exception handling in InfoCommand

63f5f8b

Fix merge

e90b51c

PaulusParssinen commented Apr 17, 2024

View reviewed changes

libs/server/ACL/ACLParser.cs Outdated Show resolved Hide resolved

libs/server/Resp/Bitmap/BitmapCommands.cs Outdated Show resolved Hide resolved

libs/server/Resp/Bitmap/BitmapCommands.cs Outdated Show resolved Hide resolved

Remove unnecessary case insensitive comparisons

cc17c4f

* StringComparison.Ordinal gets unrolled by JIT

lmaas suggested changes Apr 18, 2024

View reviewed changes

lmaas and others added 5 commits April 17, 2024 17:57

Merge branch 'main' into avoid-ascii-re-encoding

265e873

Remove empty entries from tokenized ACL rule input

4942658

Make ACL op keywords case insensitive per the PR feedback

9a0f2d6

Check that Utf8Parser.TryParse consumes entire input

a996583

Misc. adjustments

b32f60a

* Comparisons strings to uppercase * Formatting adjustments * Few unnecessary using directives in the files which PR touches

PaulusParssinen requested a review from lmaas April 18, 2024 11:14

Merge branch 'main' into avoid-ascii-re-encoding

613f02f

lmaas suggested changes Apr 19, 2024

View reviewed changes

libs/cluster/Server/ClusterConfig.cs Outdated Show resolved Hide resolved

PaulusParssinen added 4 commits April 20, 2024 14:26

Fix public field casing in Worker.cs

18529d3

Make Cluster node id comparisons case insensitive

4d1c9a4

Make local node getters properties

a0d718d

* Makes it clear that the methods don't do extra "work"

dotnet format

e6e29b7

PaulusParssinen requested a review from lmaas April 20, 2024 11:50

lmaas approved these changes Apr 20, 2024

View reviewed changes

lmaas merged commit 03611e1 into microsoft:main Apr 20, 2024
21 checks passed

PaulusParssinen deleted the avoid-ascii-re-encoding branch April 20, 2024 19:51

github-actions bot locked and limited conversation to collaborators Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command argument parsing improvements #255

Command argument parsing improvements #255

PaulusParssinen commented Apr 7, 2024 •

edited

Loading

badrishc commented Apr 16, 2024 •

edited

Loading

lmaas left a comment

lmaas left a comment

Command argument parsing improvements #255

Command argument parsing improvements #255

Conversation

PaulusParssinen commented Apr 7, 2024 • edited Loading

Culture invariant parsing

Enums

Misc.

badrishc commented Apr 16, 2024 • edited Loading

lmaas left a comment

Choose a reason for hiding this comment

lmaas left a comment

Choose a reason for hiding this comment

PaulusParssinen commented Apr 7, 2024 •

edited

Loading

badrishc commented Apr 16, 2024 •

edited

Loading