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

originalPositionFor() API fix for indexed maps: faster due to not executing a useless=buggy comparison near the end in binary search #392

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GerHobbelt
Copy link
Contributor

binary-search fixes:

  • always use the NEEDLE as the first argument for the comparison function. Otherwise you get very nasty comparisons (which don't hurt, but are utterly useless) like these in indexed maps at least:
    watch for the compareValue/compareValue2 = NaN log lines below: this is due to a useless (due to way it was invoked!) call to aCompare at the end of binary-search.

The log below is a heavily instrumented originalPositionFor() API test run which was failing for indexed maps.

originalPositionFor: { generatedLine: 2, generatedColumn: 3 }

# search round 1
binarySearch SECTIONS: { needle: { generatedLine: 2, generatedColumn: 3 },
'section.generatedOffset': { generatedLine: 1, generatedColumn: 1 },
compareCheck: 1,
compareValue2: 3 }

# search round 2
binarySearch SECTIONS: { needle: { generatedLine: 2, generatedColumn: 3 },
'section.generatedOffset': { generatedLine: 1, generatedColumn: 3 },
compareCheck: 1,
compareValue2: 1 }

# while loop round 1; always fails thanks to NaNs.
# That ain't no needle being fed to it!
binarySearch SECTIONS: { needle:
 { generatedOffset: { generatedLine: 1, generatedColumn: 3 },
   consumer:
    BasicSourceMapConsumer {
      _sourceLookupCache: Map {},
      _names: [Object],
      _sources: [Object],
      _absoluteSources: [Object],
      sourceRoot: null,
      sourcesContent: null,
      _mappings: 'AAAAA,CCCCC',
      _sourceMapURL: undefined,
      file: null,
      _computedColumnSpans: false,
      _mappingsPtr: 1114216,
      _wasm: [Object] } },
'section.generatedOffset': { generatedLine: 1, generatedColumn: 1 },
compareCheck: NaN,
compareValue2: NaN }
binarySearch SECTIONS --> found @ index: { sectionIndex: 1,
section:
 { generatedOffset: { generatedLine: 1, generatedColumn: 3 },
   consumer:
    BasicSourceMapConsumer {
      _sourceLookupCache: Map {},
      _names: [Object],
      _sources: [Object],
      _absoluteSources: [Object],
      sourceRoot: null,
      sourcesContent: null,
      _mappings: 'AAAAA,CCCCC',
      _sourceMapURL: undefined,
      file: null,
      _computedColumnSpans: false,
      _mappingsPtr: 1114216,
      _wasm: [Object] } } }

# and this is what the API gives back. Unexpected, very probably wrong,
# but that's for another PR. The key element here is the faulty use/code
# of aCompare/search.
#
# 'rv' = Return Value
originalPositionFor: { line: 2,
column: 3,
rv: { source: null, line: null, column: null, name: null } }
  • kill the fixed 3rd undocumented argument for the compare function. It's been with us since (SHA-1: 5241b06 * Always return the smallest element when there is more than one match) and has no function / is unused.

- always use the NEEDLE as the first argument for the comparison function. Otherwise you get very nasty comparisons (which don't hurt, but are utterly useless) like these in *indexed maps* at least:
  watch for the compareValue/compareValue2 = **NaN** log lines below: this is due to a useless (due to way it was invoked!) call to `aCompare` at the end of binary-search.
  The log below is a heavily instrumented `originalPositionFor()` API test run which was failing for indexed maps.
  ```
originalPositionFor: { generatedLine: 2, generatedColumn: 3 }

# search round 1
binarySearch SECTIONS: { needle: { generatedLine: 2, generatedColumn: 3 },
  'section.generatedOffset': { generatedLine: 1, generatedColumn: 1 },
  compareCheck: 1,
  compareValue2: 3 }

# search round 2
binarySearch SECTIONS: { needle: { generatedLine: 2, generatedColumn: 3 },
  'section.generatedOffset': { generatedLine: 1, generatedColumn: 3 },
  compareCheck: 1,
  compareValue2: 1 }

# while loop round 1; always fails thanks to NaNs.
# That ain't no needle being fed to it!
binarySearch SECTIONS: { needle:
   { generatedOffset: { generatedLine: 1, generatedColumn: 3 },
     consumer:
      BasicSourceMapConsumer {
        _sourceLookupCache: Map {},
        _names: [Object],
        _sources: [Object],
        _absoluteSources: [Object],
        sourceRoot: null,
        sourcesContent: null,
        _mappings: 'AAAAA,CCCCC',
        _sourceMapURL: undefined,
        file: null,
        _computedColumnSpans: false,
        _mappingsPtr: 1114216,
        _wasm: [Object] } },
  'section.generatedOffset': { generatedLine: 1, generatedColumn: 1 },
  compareCheck: NaN,
  compareValue2: NaN }
binarySearch SECTIONS --> found @ index: { sectionIndex: 1,
  section:
   { generatedOffset: { generatedLine: 1, generatedColumn: 3 },
     consumer:
      BasicSourceMapConsumer {
        _sourceLookupCache: Map {},
        _names: [Object],
        _sources: [Object],
        _absoluteSources: [Object],
        sourceRoot: null,
        sourcesContent: null,
        _mappings: 'AAAAA,CCCCC',
        _sourceMapURL: undefined,
        file: null,
        _computedColumnSpans: false,
        _mappingsPtr: 1114216,
        _wasm: [Object] } } }

# and this is what the API gives back. Unexpected, very probably wrong,
# but that's for another PR. The key element here is the faulty use/code
# of aCompare/search.
#
# 'rv' = Return Value
originalPositionFor: { line: 2,
  column: 3,
  rv: { source: null, line: null, column: null, name: null } }
  ```

- kill the fixed 3rd *undocumented* argument for the compare function. It's been with us since (SHA-1: 5241b06 * Always return the smallest element when there is more than one match) and has no function / is **unused**.
@coveralls
Copy link

coveralls commented Jun 18, 2019

Pull Request Test Coverage Report for Build 554

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.385%

Totals Coverage Status
Change from base Build 550: 0.0%
Covered Lines: 824
Relevant Lines: 911

💛 - Coveralls

2 similar comments
@coveralls
Copy link

Pull Request Test Coverage Report for Build 554

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.385%

Totals Coverage Status
Change from base Build 550: 0.0%
Covered Lines: 824
Relevant Lines: 911

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 554

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.385%

Totals Coverage Status
Change from base Build 550: 0.0%
Covered Lines: 824
Relevant Lines: 911

💛 - Coveralls

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.

2 participants