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

Create a Table Row Type and expose as a Vector on In-Memory Table with .rows property #3827

Merged
merged 14 commits into from
Oct 26, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Oct 24, 2022

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/182307026

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@radeusgd radeusgd self-assigned this Oct 24, 2022
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

The implementation would have to be made ready to run on fast path. Anyway, I am not very excited to see this duck typing idea overall...

@radeusgd radeusgd force-pushed the wip/radeusgd/table-rows-182307026 branch from ee05a4a to 70ba509 Compare October 25, 2022 18:42
@radeusgd radeusgd marked this pull request as ready for review October 25, 2022 18:43
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I believe the ArrayProxy implementation is correct. I suggest to expand the VectorBenchmarks class with ArrayProxy case and compare it compiles well. Use ProxyList benchmark as an inspiration.

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

libs part looks good - happy for this to go in once @JaroslavTulach and @hubertp happy with the built-in part.

@radeusgd radeusgd force-pushed the wip/radeusgd/table-rows-182307026 branch from 658464a to c80872c Compare October 26, 2022 09:16
@radeusgd
Copy link
Member Author

radeusgd commented Oct 26, 2022

I added the benchmarks as requested, for comparison I added the AbstractList benchmark @JaroslavTulach created too.

The results on my machine are following:

Benchmark Time [μs/op]
averageAbstractList 119.85
averageOverArrayProxy 46.87
averageOverArray 18.24
averageOverPolyglotArray 18.05
averageOverPolyglotVector 16.74
averageOverSlice 17.25

So the proxy is roughly 2.5x slower than direct access (probably due to the additional layer of indirection), but still almost 2.6x faster than the polyglot AbstractList.

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 26, 2022
@mergify mergify bot merged commit bb29833 into develop Oct 26, 2022
@mergify mergify bot deleted the wip/radeusgd/table-rows-182307026 branch October 26, 2022 11:21
@JaroslavTulach
Copy link
Member

averageOverArrayProxy 46.87
averageOverArray 18.24
So the proxy is roughly 2.5x slower than direct access

That is very slow. It should be on par with direct access. I'd try the benchmark without the bounds check. E.g. after removing:

    if (index >= length || index < 0) {
      throw InvalidArrayIndexException.create(index);
    }

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

nit regarding range check

@ExportMessage
public Object readArrayElement(long index, @CachedLibrary(limit = "3") InteropLibrary interop)
throws UnsupportedMessageException, InvalidArrayIndexException {
if (index >= length || index < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's correct or even necessary? If it is outside of the range but you tried to access it you should get UnsupportedMessageException? Right @JaroslavTulach ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is necessary to get the semantics that one would normally expect.

If I remove it, the following test:

        Test.specify "should correctly delegate to the callback" <|
            arr = Array_Proxy.new 3 (ix -> ix + 10)
            arr.length . should_equal 3
            arr.at 0 . should_equal 10
            arr.at 1 . should_equal 11
            arr.at 2 . should_equal 12
            arr.at 3 . should_fail_with Index_Out_Of_Bounds_Error_Data

will fail like so:

Array_Proxy:  [168ms]
    - [FAILED] should correctly delegate to the callback [103ms]
        Reason: Expected an error Constructor<Index_Out_Of_Bounds_Error_Data> but no error occurred, instead got: 13 (at /home/radeusgd/NBO/enso/test/Tests/src/Data/Array_Proxy_Spec.enso:20:13-70).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants