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

Improve serializing/string conversions + some other opportunities to optimize code #102

Open
nayr7 opened this issue Jun 14, 2023 · 1 comment
Labels
enhancement New feature or request postponed postponed for later

Comments

@nayr7
Copy link
Contributor

nayr7 commented Jun 14, 2023

Consider the following snippets:

Example 1:

## objects.nim


proc newMessage*(data: JsonNode): Message =
    result = data.`$`.fromJson(Message)

# or ...

proc `[]=`(obj: ref object, fld: string, val: JsonNode) =
    for name, field in obj[].fieldPairs:
        if name == fld:
            field = ($val).fromJson(typeof(field))

# etc ...

and just about anything that use fromJson but cannot take in JsonNodes

Example 2:

# it's cool that we can do something clean like this in Nim:
        let row = newActionRow @[
            newButton(label = "Lose ?", idOrUrl = "lost", style = Danger),
            newButton(label = "Win ?", idOrUrl = "won", style = Primary)
        ]

... but we can do even better:

# notice the lack of `@`, we're using arrays instead of seq == statically allocated array == more speed !
        let row = newActionRow [
            newButton(label = "Lose ?", idOrUrl = "lost", style = Danger),
            newButton(label = "Win ?", idOrUrl = "won", style = Primary)
        ]

... which brings us to

openArray and other stringing shenanigans :

  • Using more sink and openArray[char] in the code when possible
  • Using procs that can accept JsonNode instead of converting them to string which is surely a way too costly operation for functions that run as often as newMessage

profiling as we go

The main trap of optimizing stuff is that we think we are making things faster when in fact we are making them slower, every change must come with microbenchmark to ensure we are not shooting ourselves in the foot

@krisppurg krisppurg added enhancement New feature or request postponed postponed for later labels Jun 14, 2023
@ire4ever1190
Copy link
Contributor

Issue with openArray is that it cannot be a parameter for async procs (Since it cant be guaranteed that the array/seq it is pointing to will still be alive). Could use something like an Iterable concept that kinda achieves the same effect (Although it would increase the binary size if used with lots of different array sizes).

The reason for the string conversion is because of the use of jsony, could switch to jsonutils. While it isn't faster in benchmarks, not needing to convert to/from strings could make it faster (Another benefit, jsonutils has better error messages).

Has with most API clients, biggest slowdown will be network. Would be good to analyse refactoring requester.nim to use a pool of clients instead of creating a new one everytime to see if it gives lower latency

Rest of the suggestions seem good though 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request postponed postponed for later
Projects
None yet
Development

No branches or pull requests

3 participants