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

Add Table.take and Table.drop functions to In-Memory table #3647

Merged
merged 15 commits into from
Aug 26, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Aug 12, 2022

Pull Request Description

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

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 force-pushed the wip/radeusgd/in-memory-table-take-drop-182307347 branch 4 times, most recently from 166d31d to c60f4e1 Compare August 24, 2022 11:20
@radeusgd radeusgd self-assigned this Aug 24, 2022
@radeusgd radeusgd marked this pull request as ready for review August 24, 2022 11:21
@radeusgd radeusgd requested a review from hubertp August 24, 2022 11:23
@radeusgd radeusgd force-pushed the wip/radeusgd/in-memory-table-take-drop-182307347 branch 2 times, most recently from a27d51b to cb29c33 Compare August 25, 2022 09:34
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.

As discussed a couple of minor comments:

  • Lets comment why we are keeping Sample in Vector
  • Lets add Index_Sub_Range to Standard.Base and remove the other imports

Worth testing performance of Interface vs Record

@radeusgd
Copy link
Member Author

I added the comment.

Lets add Index_Sub_Range to Standard.Base and remove the other imports

As discussed, I cannot do that due to a bug. We can do it once the bug is resolved.

@radeusgd
Copy link
Member Author

Worth testing performance of Interface vs Record

As requested, I wrote the following code:

package org.enso.base;

import java.util.List;

public class Foo {
	public record TestRecord(long x, long y) {}
	public interface TestInterface {
		long x();
		long y();
	}

	public static long record_product(List<TestRecord> records) {
		long result = 0;
		for (TestRecord record : records) {
			result += record.x * record.y;
		}
		return result;
	}

	public static long interface_product(List<TestInterface> interfaces) {
		long result = 0;
		for (TestInterface iface : interfaces) {
			result += iface.x() * iface.y();
		}
		return result;
	}
}
from Standard.Base import all
import Standard.Test.Bench
polyglot java import org.enso.base.Foo

n = 500000

type My_Atom x y

sum_with_conversion list =
    converted_list = list.map atom->
        Foo.TestRecord.new atom.x atom.y
    Foo.record_product converted_list.to_array

sum_ifaces list =
    Foo.interface_product list.to_array

main =
    list = (0.up_to n).map (x -> My_Atom (x % 100 + 10) (x % 197 + 12))
    reps = 10
    Bench.measure (sum_with_conversion list) "sum records with conversion" reps reps
    Bench.measure (sum_ifaces list) "sum interfaces" reps reps

The results on my machine were following:

sum records with conversion/iteration:0: 502.26ms
sum records with conversion/iteration:1: 123.61ms
sum records with conversion/iteration:2: 138.55ms
sum records with conversion/iteration:3: 109.20ms
sum records with conversion/iteration:4: 81.37ms
sum records with conversion/iteration:5: 113.67ms
sum records with conversion/iteration:6: 81.75ms
sum records with conversion/iteration:7: 102.50ms
sum records with conversion/iteration:8: 80.69ms
sum records with conversion/iteration:9: 112.75ms
sum interfaces/iteration:0: 223.75ms
sum interfaces/iteration:1: 182.16ms
sum interfaces/iteration:2: 175.16ms
sum interfaces/iteration:3: 166.94ms
sum interfaces/iteration:4: 167.23ms
sum interfaces/iteration:5: 172.89ms
sum interfaces/iteration:6: 171.71ms
sum interfaces/iteration:7: 169.90ms
sum interfaces/iteration:8: 168.23ms
sum interfaces/iteration:9: 167.91ms

So, it seems that after warm-up the conversion to records is actually faster than the interfaces, but it is slower in the first iterations. This makes the decision unclear - in Enso we often want the first runs to be fast too.
Overall still I guess we should move to records, what do you think @jdunkerley ?

@radeusgd radeusgd force-pushed the wip/radeusgd/in-memory-table-take-drop-182307347 branch 2 times, most recently from a32d691 to 9cca29e Compare August 26, 2022 09:37
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Aug 26, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/in-memory-table-take-drop-182307347 branch from 9cca29e to ffa4454 Compare August 26, 2022 19:01
@mergify mergify bot merged commit d7ebc4a into develop Aug 26, 2022
@mergify mergify bot deleted the wip/radeusgd/in-memory-table-take-drop-182307347 branch August 26, 2022 19:41
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.

None yet

2 participants