[Skylark] Make range function lazy. #5240

ttsugriy · 2018-05-23T03:43:19Z

range used to use MutableList which would eagerly allocate an array list with
all range elements, which is not efficient for very large ranges or when only
a small number of its elements are used.

This implementation uses a constant amount of RAM and computes a value for
each requested index.

For the following Skylark snippet:

def check_content(t):
  if t == []:
    return t
  return False

def modulo(n):
  return n % 797

N = 10000000

[check_content(i) for i in range(N)]
[check_content(i) for i in range(N)]
[modulo(i) for i in range(N)]
[modulo(i) for i in range(N)]

the total runtime goes from

$ time bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl
bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl  93.09s user 1.67s system 316% cpu 29.930 total

to

$ time bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl
bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl  31.45s user 0.86s system 179% cpu 17.974 total

which reflects the reduced system time (fewer allocations) and performance.

ttsugriy

for improved iteration performance it also makes sense to implement a specialized iterator to make computation of the next element cheaper and avoid unnecessary checks that AbstractList.Itr does

johnynek · 2018-05-23T20:19:16Z

This is exciting!

laurentlb · 2018-05-24T13:20:05Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+      for (int pos : sliceIndices) {
+        builder.add(this.get(pos));
+      }
+      return MutableList.createImmutable(builder.build());


Maybe implement it lazily?
https://github.com/google/skylark/blob/master/library.go#L888

absolutely, I meant to leave a TODO for this though, since I didn't want to complicate this change even more. Also, I believe that all slice implementations should be made lazy and more efficient, since currently MutableList and Tuple both create news collections, which can be avoided. I'll try to create a new abstraction for this.

A TODO is fine for me.
Be careful with lazy slices, e.g.

a = [1, 2, 3] b = a[1:] a[2] = 5 b # [2, 3]

laurentlb · 2018-05-24T13:21:06Z

src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java

@@ -486,6 +486,7 @@ public void testRange() throws Exception {
        .testStatement("str(range(5, 0, -1))", "[5, 4, 3, 2, 1]")
        .testStatement("str(range(5, 0, -10))", "[5]")
        .testStatement("str(range(0, -3, -2))", "[0, -2]")
+        .testStatement("str(range(3)[1:2])", "[1]")


Please add more tests combining range and slicing
https://github.com/google/skylark/blob/master/testdata/builtins.sky#L94

laurentlb · 2018-05-24T13:23:02Z

Thanks a lot! It looks good.

I'll be away a few days, deferring to Jon for the approval

ttsugriy · 2018-05-24T18:08:07Z

added TODO and tests, thanks for the pointers @laurentlb!

brandjon · 2018-05-25T02:11:57Z

Thanks for the PR. I see a couple issues.

The first is that we should try to preserve backward compatibility until we're ready to introduce a behavioral change in a controlled way (i.e. with an --incompatible_* change flag). For range(), the current behavior is that it returns a list, but in the future we want it to return a view. The difference might be observed when the user does any of the following on the result of range():

takes its str()/repr() or type()
compares it to other range() results or to normal lists/tuples, with respect to either equality or order
mutates it
concatenates it with lists or tuples
takes a slice of it and then does any of the above

I've added a quick test to check some of these (33f08e7). In any case, ideally they should all be maintained across any semantics-preserving optimization PR like this one.

(We have an old unimplemented internal design doc on the desired view-like behavior of range(). We really should publish that and revisit the design so we all agree on what the end state will be. Created #5264 to track.)

The second issue is the class hierarchy of non-list-non-tuple sequences. The pattern we're trying to evolve toward is that each distinct user-visible Skylark type corresponds to a class with a @SkylarkModule annotation. For example, SkylarkList is "sequence", MutableList is "list", Tuple is "tuple". A subclass should not have its own @SkylarkModule annotation unless its type is observably different (e.g., a different type() or additional @SkylarkCallable API). See the very recent explanation added in cdf5a07.

In this PR, given the backward compatibility constraints that range() objects should (for now) appear to be lists, this means that ideally RangeList and MutableList should inherit from a common interface (ListLike?) that is a SkylarkList, and the "list" @SkylarkModule should be pushed up to this interface. range() objects should not be concatable with tuples because lists are not. I think that means we can eliminate TupleLike.

To make progress, I suggest we make sure we have test coverage for range-list-compatibility on the behaviors I listed above (we might already be good, I haven't checked in detail), and then modify this PR to comply.

brandjon · 2018-05-25T02:53:52Z

I suggest we make sure we have test coverage for range-list-compatibility on the behaviors I listed above

In fact, I'll do that now.

brandjon · 2018-05-25T14:38:12Z

Done in 585418d.

ttsugriy · 2018-05-29T20:50:24Z

Thanks for such a detailed information, @brandjon! I'll rebase and see if any of the tests you've kindly added are broken. I'll also happily remove the TupleLike interface :)

ttsugriy · 2018-05-29T23:20:52Z

after taking another look at the code, I think that maybe it would be better to just introduce the --incompatible_use_range_type flag that would be used to decide which implementation to use and add @SkylarkModule with a range type to the RangeList class, since implementing append for RangeList is going to be messy. What do you think, @brandjon?

ttsugriy · 2018-05-30T02:40:25Z

I updated the change to use --incompatible_range_type to control whether the new implementation of range should be used and added corresponding tests. For better compatibility with Python 3 though, I guess I still need to reimplement getSlice to return range instead of list...

laurentlb · 2018-07-04T16:30:54Z

Looks good to me, thanks!

brandjon · 2018-07-05T19:59:42Z

src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java

-            }
+            // simplify handling of a case like range(-2) or its equivalent range(-2, 0)
+            // by turning it into range(0, 0)
+            stop = Math.max(start, stop);


Why? Shouldn't this be ok albeit degenerate?

If you do this for step > 0, you probably also want to set stop = Math.min(start, stop) when step < 0.

after playing with Python interpreter I realized that Python actually allows this and displays the range the way it was constructed, so this change is not good. I'm going to remove it.

brandjon · 2018-07-05T20:01:23Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+        }
+      }
+
+      private static int computeSize(int start, int stop, int step) {


Since you're abs()ing both the difference and step, this'll report non-zero size when the range should actually be empty due to step having the wrong sign.

I've decided to implement it similarly to https://github.com/python/cpython/blob/09bb918a61031377d720f1a0fa1fe53c962791b6/Objects/rangeobject.c#L144-L210 to make it easier to port CPython behavior

brandjon · 2018-07-05T20:21:50Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+        final int length = Math.abs(stop - start);
+        final int absolute_step = Math.abs(step);
+        // round up (length / absolute_step) without using floats
+        return 1 + ((length - 1) / absolute_step);


If length is 0 but the absolute step is bigger than 1, the quotient will 0 and the returned result will be 1.

replaced with logic from https://github.com/python/cpython/blob/09bb918a61031377d720f1a0fa1fe53c962791b6/Objects/rangeobject.c#L144-L210

brandjon · 2018-07-05T20:28:18Z

FYI usually we try not to modify incompatible change flags too much, so if you want to make getSlice return a range as well, I'd either do it before we flip this flag to true or else use a second flag.

ttsugriy · 2018-07-05T21:05:01Z

@brandjon, yeah, I just started working on it. I hope to get working slices in the next day or two if my oncall rotation is not too crazy :)

brandjon · 2018-07-05T21:23:56Z

I forgot, when you add lazy slicing, can you please also document the incompatible change flag? It goes in site/docs/skylark/backward-compatibility.md.

ttsugriy · 2018-07-05T21:44:27Z

Will do, thanks for the pointers @brandjon! I've also updated PR to include lazy range slices.

ttsugriy · 2018-07-05T21:52:15Z

added a section to backwards compatibility doc.

brandjon · 2018-07-06T17:20:17Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+      RangeListView.Slice slice = RangeListView.Slice.from(size(), start, end, step, loc);
+      int substep = slice.step * this.step;
+      int substart = get(slice.start);
+      int substop = get(slice.stop);


It's really confusing which indices are inclusive and exclusive in the new code. A comment or two might help. The simplest interpretation is that start and stop are consistently inclusive and exclusive, throughout. In that case, slice.stop is not necessarily a valid input to get(), when the slice's endpoint is the same as the original range's.

Excellent point! :) I've updated code to use [start, stop) convention everywhere

brandjon · 2018-07-06T17:21:19Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+      @Override
+      public Integer get(int index) {
+        int value = start + step * index;
+        if ((step > 0 && value > stop) || (step < 0 && value < stop)) {


If stop is exclusive, shouldn't it be if value >= stop and value <= stop?

Then again, why do the computation on values at all? Why not just test if index < 0 || index >= size?

great point! I didn't realize that index that is passed here is already normalized

brandjon · 2018-07-06T17:24:07Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+       * Python version: https://github.com/python/cpython/blob/09bb918a61031377d720f1a0fa1fe53c962791b6/Objects/rangeobject.c#L144
+       */
+      private static int computeSize(int start, int stop, int step) {
+        int low;


In light of the inclusion/exclusion confusion elsewhere, can you add a comment indicating that this is a half-open interval and it doesn't matter which var represents the included endpoint?

brandjon · 2018-07-06T17:27:54Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+            throw new EvalException(loc, "slice step cannot be zero");
+          }
+
+          int upper;


Comment that one endpoint is inclusive and the other exclusive.

brandjon · 2018-07-06T17:28:34Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+          int lower;
+          if (step < 0) {
+            lower = -1;
+            upper = length + lower;


length - 1 is clearer than length - lower.

I was trying to be as close to CPython implementation as possible, but in this particular case I think the usage of + length is somewhat accidental and only hurts readability, so applied your suggestion.

ttsugriy · 2018-07-06T20:34:32Z

thanks for the awesome suggestions, @brandjon! I've addressed all of them and added a couple of test cases. Please take another look.

ttsugriy · 2018-07-10T18:16:40Z

@brandjon, could you please take another look?

laurentlb · 2018-07-11T12:30:35Z

Sorry for the delay - @brandjon is on leave.
I think the PR is good to go, I'm importing it. If Jon wanted more changes, we can do that later (because the change is behind a flag).

Thanks a lot for your contribution, it's very exciting!

ttsugriy · 2018-07-11T15:28:41Z

thank you, @laurentlb! I'll be happy to address any concerns Jon may have once he's back :)

range used to use MutableList which would eagerly allocate an array list with all range elements, which is not efficient for very large ranges or when only a small number of its elements are used. This implementation uses a constant amount of RAM and computes a value for each requested index.

c-parsons · 2018-07-12T22:07:04Z

src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java

@@ -486,6 +486,7 @@ public void testRange() throws Exception {
        .testStatement("str(range(5, 0, -1))", "[5, 4, 3, 2, 1]")


Add a test for mult, which is also now disabled?

I thought I had one... I guess I was wrong... :)

c-parsons · 2018-07-12T22:09:40Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+              + "range(10)[::2]  # range(0, 10, 2)\n"
+              + "range(10)[3:0:-1]  # range(3, 0, -1)</pre>"
+              + "Ranges are immutable, as in Python 3.")
+  public static final class RangeList extends SkylarkList<Integer> {


I have a preference for these new classes be in separate files. Please :)

yeah, I just wasn't sure what's the convention, since the rest of SkylarkList implementations are inlined :)

c-parsons · 2018-07-12T22:10:24Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+    private static class RangeListView extends AbstractList<Integer> {
+
+      /** Iterator for increasing sequences. */
+      private static class IncreasingIterator extends UnmodifiableIterator<Integer> {


In principle I thought it was fine that we were separating IncreasingIterator and DecreasingIterator for clarity purposes, but then it turns out that these classes are identical except for hasNext().

Perhaps we should combine them, and hasNext() can be return (step > 0) ? : cursor < stop : cursor > stop;

that's how I started, but then started prematurely optimizing to avoid extra branches :)

c-parsons · 2018-07-12T22:10:37Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+      }
+    }
+
+    public MutableList<Integer> toMutableList(Environment env) {


javadoc? (at least to describe the ramifications of doing this)javadoc

c-parsons · 2018-07-12T22:10:44Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java

+      return MutableList.copyOf(env, contents);
+    }
+
+    public static RangeList of(int start, int stop, int step) {


c-parsons

(apologies, meant to request changes on my past review. Marking that for posterity)

ttsugriy · 2018-07-13T05:06:02Z

addressed all concerns, @c-parsons, please take another look.

c-parsons

A couple very minor comments -- this is looking good!

c-parsons · 2018-07-16T18:38:54Z

src/main/java/com/google/devtools/build/lib/syntax/RangeList.java

+  /** Provides access to range elements based on their index. */
+  private static class RangeListView extends AbstractList<Integer> {
+
+    /** Iterator for increasing sequences. */


Fix javadoc

c-parsons · 2018-07-16T18:41:17Z

src/main/java/com/google/devtools/build/lib/syntax/RangeList.java

+  }
+
+  /**
+   * @return A materialized version of the range that can be used as a <pre>list</pre> type.<p>


Nit: Throughout bazel we put @return at the end of javadoc blocks

…e_type.

ttsugriy · 2018-07-16T22:56:43Z

addressed all of the comments above, please take another look, @c-parsons :)

c-parsons

LGTM. Thanks for your work!

ttsugriy · 2018-07-17T00:10:16Z

Thank you for the review, @c-parsons!

range used to use MutableList which would eagerly allocate an array list with all range elements, which is not efficient for very large ranges or when only a small number of its elements are used. This implementation uses a constant amount of RAM and computes a value for each requested index. For the following Skylark snippet: ``` def check_content(t): if t == []: return t return False def modulo(n): return n % 797 N = 10000000 [check_content(i) for i in range(N)] [check_content(i) for i in range(N)] [modulo(i) for i in range(N)] [modulo(i) for i in range(N)] ``` the total runtime goes from ``` $ time bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl 93.09s user 1.67s system 316% cpu 29.930 total ``` to ``` $ time bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl bazel-bin/src/main/java/com/google/devtools/skylark/Skylark test.bzl 31.45s user 0.86s system 179% cpu 17.974 total ``` which reflects the reduced system time (fewer allocations) and performance. Closes bazelbuild#5240. PiperOrigin-RevId: 204918577

googlebot added the cla: yes label May 23, 2018

ttsugriy force-pushed the range branch from 7e2b804 to f0fcfd6 Compare May 23, 2018 07:06

ttsugriy commented May 23, 2018

View reviewed changes

ttsugriy force-pushed the range branch from f0fcfd6 to a97daf3 Compare May 23, 2018 17:41

laurentlb requested a review from brandjon May 24, 2018 13:22

laurentlb reviewed May 24, 2018

View reviewed changes

ttsugriy force-pushed the range branch from 252a983 to 13f5a11 Compare May 24, 2018 18:07

ttsugriy force-pushed the range branch 2 times, most recently from e869eec to be1f5db Compare May 24, 2018 18:22

ttsugriy force-pushed the range branch from be1f5db to ae7c63d Compare May 30, 2018 02:38

ttsugriy force-pushed the range branch from ae7c63d to f82e6e6 Compare May 30, 2018 02:45

ttsugriy force-pushed the range branch from f82e6e6 to 28fc675 Compare June 8, 2018 21:02

ttsugriy force-pushed the range branch from 28fc675 to 71cce8b Compare June 26, 2018 22:12

hlopko assigned laurentlb Jun 28, 2018

brandjon requested changes Jul 5, 2018

View reviewed changes

ttsugriy force-pushed the range branch from 71cce8b to 97ccfee Compare July 5, 2018 20:47

brandjon approved these changes Jul 5, 2018

View reviewed changes

ttsugriy force-pushed the range branch from 97ccfee to 7ceac03 Compare July 5, 2018 21:43

ttsugriy force-pushed the range branch from 7ceac03 to f80f0ee Compare July 5, 2018 21:52

brandjon requested changes Jul 6, 2018

View reviewed changes

ttsugriy force-pushed the range branch 2 times, most recently from 25c6201 to 5d14097 Compare July 6, 2018 20:33

ttsugriy force-pushed the range branch from 5d14097 to dc6ec07 Compare July 6, 2018 20:42

ttsugriy mentioned this pull request Jul 9, 2018

[Skylark] Extract slice utility methods into separate methods. #5302

Closed

ttsugriy added 2 commits July 12, 2018 12:52

[Skylark] Implement specialized RangeListView iterators.

0c25813

ttsugriy force-pushed the range branch from dc6ec07 to 3ee7804 Compare July 12, 2018 19:52

c-parsons self-requested a review July 12, 2018 22:01

c-parsons reviewed Jul 12, 2018

View reviewed changes

c-parsons requested changes Jul 12, 2018

View reviewed changes

ttsugriy force-pushed the range branch 2 times, most recently from 820a9b2 to c598d2a Compare July 13, 2018 05:05

c-parsons requested changes Jul 16, 2018

View reviewed changes

[Skylark] Control whether range type is used with --incompatible_rang…

aa7d616

…e_type.

ttsugriy force-pushed the range branch from c598d2a to aa7d616 Compare July 16, 2018 22:55

c-parsons approved these changes Jul 16, 2018

View reviewed changes

bazel-io closed this in 9de215d Jul 17, 2018

		@@ -486,6 +486,7 @@ public void testRange() throws Exception {
		.testStatement("str(range(5, 0, -1))", "[5, 4, 3, 2, 1]")

[Skylark] Make range function lazy. #5240

[Skylark] Make range function lazy. #5240

Conversation

ttsugriy commented May 23, 2018 • edited Loading

ttsugriy left a comment

Choose a reason for hiding this comment

johnynek commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentlb commented May 24, 2018

ttsugriy commented May 24, 2018

brandjon commented May 25, 2018

brandjon commented May 25, 2018

brandjon commented May 25, 2018

ttsugriy commented May 29, 2018

ttsugriy commented May 29, 2018

ttsugriy commented May 30, 2018

laurentlb commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandjon commented Jul 5, 2018

ttsugriy commented Jul 5, 2018

brandjon commented Jul 5, 2018

ttsugriy commented Jul 5, 2018

ttsugriy commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttsugriy commented Jul 6, 2018

ttsugriy commented Jul 10, 2018

laurentlb commented Jul 11, 2018

ttsugriy commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-parsons left a comment

Choose a reason for hiding this comment

ttsugriy commented Jul 13, 2018

c-parsons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttsugriy commented Jul 16, 2018

c-parsons left a comment

Choose a reason for hiding this comment

ttsugriy commented Jul 17, 2018

ttsugriy commented May 23, 2018 •

edited

Loading