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

refactor hash in JS backend #16863

Merged
merged 6 commits into from
Jan 30, 2021
Merged

refactor hash in JS backend #16863

merged 6 commits into from
Jan 30, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Jan 29, 2021

make hash more type-safe in JS backend

The workaround which I discussed with @timotheecour long time ago.

      const buffer = new ArrayBuffer(8);
      const floatBuffer = new Float64Array(buffer);
      const uintBuffer = new Uint32Array(buffer);
      floatBuffer[0] = `x`;
      `result` = BigInt(uintBuffer[0]) + (BigInt(uintBuffer[1]) << 32n);

lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
lib/pure/hashes.nim Outdated Show resolved Hide resolved
Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM again.

these performance tests confirm what I was suspecting:

import std/jsbigints
import std/times

type Hash = int

proc fn(a: JsBigInt) =
  when defined case1:
    let maxSafeInteger = big"0x1FFFFFFFFFFFFF"
    let a1 = cast[Hash](toNumber((a and maxSafeInteger))) and cast[Hash](0xFFFFFFFF)
    let a2 = cast[Hash](toNumber(wrapToInt(a, 32)))
    doAssert a1 == a2

  elif defined case2:
    let maxSafeInteger = big"0x1FFFFFFFFFFFFF"
    let a1 = cast[Hash](toNumber((a and maxSafeInteger))) and cast[Hash](0xFFFFFFFF)
    doAssert a1 != 12

  elif defined case3:
    let maxSafeInteger = (big"1" shl big"53") - big"1"
    let a1 = cast[Hash](toNumber((a and maxSafeInteger))) and cast[Hash](0xFFFFFFFF)
    doAssert a1 != 12

  elif defined case4:
    let a1 = cast[Hash](toNumber(wrapToInt(a, 32)))
    doAssert a1 != 12
  else: static doAssert false

proc main()=
  let t = now()
  for i in 123..<10_000_000:
    fn(big(i))
  let t2 = now() - t
  echo t2
main()
nim r -b:js -d:danger -d:case1 main
2 seconds and 320 milliseconds
nim r -b:js -d:danger -d:case2 main
2 seconds and 73 milliseconds
nim r -b:js -d:danger -d:case3 main
4 seconds and 104 milliseconds
nim r -b:js -d:danger -d:case4 main
538 milliseconds

conclusion 1: let maxSafeInteger = (big"1" shl big"53") - big"1" isn't inlined nor optimized, so using big"0x1FFFFFFFFFFFFF" directly is best
conclusion 2: wrapToInt is indeed faster (and gives same results)

(EDIT) conclusion 3: so this PR might improve performance of hash on js, but i haven't measured that

@ringabout ringabout requested a review from Araq January 30, 2021 06:42
@Araq Araq merged commit 111092e into nim-lang:devel Jan 30, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants