Azoy / Sword

Discord library for Swift
https://azoy.github.io/Sword
MIT License
179 stars 52 forks source link

Switch from Strings to UInt64s for storing Snowflake IDs #18

Closed TellowKrinkle closed 7 years ago

TellowKrinkle commented 7 years ago

Snowflake IDs are passed as strings by the Discord API because not all languages have the ability to store UInt64s, but swift does, so there isn't much reason to store them as strings.

This is a breaking change, since any functions that cast the result of an event to a String will now have to cast it to a SnowflakeID (which is a UInt64).

Another option is to change the typealias of SnowflakeID to String, in which case it shouldn't break anything using the API while giving more flexibility in the future (since anyone who uses the SnowflakeID typealias will have their code automatically switched over if we change its definition in the future).

MrLotU commented 7 years ago

I like this

Azoy commented 7 years ago

What’s the benefit of storing as a UInt64 rather than a string? Because at the end of the day, we’re still going to have to make it a string.

TellowKrinkle commented 7 years ago

Mainly memory usage (a UInt64 is 8 bytes, a string representing a UInt64 is about 18 bytes), as well as performance (in my tests, dictionary access with an Int is about 5x faster than with a String, and dictionary access with a String that gets converted to an Int on the spot is still 2x faster than dictionary access with the String directly)

TellowKrinkle commented 7 years ago

After retesting with longer number (random UInt64s instead of 1..<10_000_000), it looks like using Strings directly (from the JSON) is slightly faster than converting to Ints first, but this should be offset by the times when we use values we've already stored (which won't need the conversion).

Here's the testing code

Results:

Memory (Arrays of 10 million of each): UInt64: 80MB String: 850MB

Dictionary Manipulation (Add/Read/Remove 10 million T -> UInt32 mappings): Int Fill: 1.288968322 seconds String -> Int Fill: 5.973246565 seconds String Fill: 7.206041881 seconds Int Read: 0.334453005 seconds String -> Int Read: 5.328426967 seconds String Read: 3.111358521 seconds Int Delete: 1.163997285 seconds String -> Int Delete: 6.394266516 seconds String Delete: 6.692069977 seconds

Azoy commented 7 years ago

I'll let you leave the if lets, but after measuring time whether or not to use dict["key"] = nil or dict.removeValue(forKey: "key") I saw that removeValue was faster in all tests I did. I'd like you to revert those changes and to make the type alias Snowflake rather than SnowflakeID. Also, right now if you want to get the snowflake you do Snowflake(dict["id"] as! String)! whereas I think it would be better to cast the id as an Int before casting to snowflake. Snowflake(dict["id"] as! Int) This removes that last force unwrap of the optional initializer.

TellowKrinkle commented 7 years ago

Have you tested recently? The reason I changed it is that when I tested, dict["key"] = nil was faster.

Azoy commented 7 years ago

Yea I tested a few hours ago.

TellowKrinkle commented 7 years ago

as! in Swift is only for changing the type of an object into a superclass/subclass. It fatal errors if you try to convert an object that is currently a String (like dict["id"]) into an Int.

Azoy commented 7 years ago

I don’t remember what the json serializer turns the id into, but if it’s a string then you can keep the string cast.

TellowKrinkle commented 7 years ago

I implemented the other two changes, but using as! Int errors my test program with Could not cast value of type '__NSCFString' (0x7fffcd31b190) to 'NSNumber' (0x7fffcd6d5d40).

Azoy commented 7 years ago

Ah yeah just keep the string cast that’s fine.