Support for boxing SliceLiteral type by njriasan · Pull Request #7714 · numba/numba

njriasan · 2022-01-08T07:35:53Z

Support boxing directly with a SliceLiteral. This fixes the example in #7707.

guilhermeleobas

Hi Nick, thanks for working on this.

stuartarchibald · 2022-06-07T13:57:02Z

Think this needs fixing:

from numba import njit

z = slice(1, 2, 3)

@njit
def foo():
    return z

foo()

via:

diff --git a/numba/core/pythonapi.py b/numba/core/pythonapi.py
index 79ca776..6dad042 100644
--- a/numba/core/pythonapi.py
+++ b/numba/core/pythonapi.py
@@ -663,7 +663,7 @@ class PythonAPI(object):
     # Concrete slice API
     #
     def slice_new(self, start, stop, step):
-        fnty = Type.function(self.pyobj, [self.pyobj, self.pyobj, self.pyobj])
+        fnty = ir.FunctionType(self.pyobj, [self.pyobj, self.pyobj, self.pyobj])
         fn = self._get_function(fnty, name="PySlice_New")
         return self.builder.call(fn, [start, stop, step])

njriasan · 2022-06-07T14:47:51Z

Thanks @stuartarchibald. I fixed the issue. In addition, I found a couple other places where Type was still used so I fixed them in this PR as well.

stuartarchibald

Thanks for the patch @njriasan and for reviewing @guilhermeleobas. I've given this a bit more of a review and left some comments in line, most should be relatively quick to resolve. Thanks again.

stuartarchibald · 2022-06-08T12:26:49Z

+    slice_lit = typ.literal_value
+    slice_fields = []
+    for field_name in ("start", "stop", "step"):
+        field_obj = getattr(slice_lit, field_name)
+        field_val = (
+            c.pyapi.make_none()
+            if field_obj is None
+            else c.pyapi.from_native_value(
+                types.literal(field_obj), field_obj, c.env_manager
+            )
+        )
+        slice_fields.append(field_val)
+    slice_val = c.pyapi.slice_new(*slice_fields)
+    for field_obj in slice_fields:
+        c.pyapi.decref(field_obj)
+    return slice_val


Something like this might also work?

@box(types.SliceLiteral) def box_slice_literal(typ, val, c): py_ctor, py_args = typ.literal_value.__reduce__() serialized_ctor = c.pyapi.serialize_object(py_ctor) serialized_args = c.pyapi.serialize_object(py_args) ctor = c.pyapi.unserialize(serialized_ctor) args = c.pyapi.unserialize(serialized_args) obj = c.pyapi.call(ctor, args) c.pyapi.decref(ctor) c.pyapi.decref(args) return obj

avoids needing to add slice_new to the pyapi too.

@stuartarchibald I'm happy to update this code if you would like but I don't understand why this change is necessary/preferred. Is there a concern that creating native literals will have gaps/performs worse?

stuartarchibald · 2022-06-08T13:12:07Z

Thanks @stuartarchibald. I fixed the issue. In addition, I found a couple other places where Type was still used so I fixed them in this PR as well.

@njriasan many thanks for fixing the other places where Type was used.

njriasan · 2022-06-14T03:05:10Z

@stuartarchibald I made all of the requested changes except the serialization change to boxing because I don't understand the benefit of the change. I'm happy to refactor the code but I'd like to understand the reason for the change.

I also made one additional change to Literal.literal_type so IntegerLiteral will show the correct error message when encountering the integer overflow issue, but I didn't see a need to add a test.

esc · 2022-06-16T17:10:57Z

I also made one additional change to Literal.literal_type so IntegerLiteral will show the correct error message when encountering the integer overflow issue, but I didn't see a need to add a test.

I think it would be good to add a test using the reproducer from #7714 (comment) -- if feasible.

esc · 2022-06-16T17:47:23Z

@stuartarchibald I made all of the requested changes except the serialization change to boxing because I don't understand the benefit of the change. I'm happy to refactor the code but I'd like to understand the reason for the change.

I managed to consult Stuart in an OOB conversation today and the idea is that the suggested code is simpler and easier to maintain. There are not many references to clean up and we don't need to add the slice_new wrapper, so we are adding less code. It may result in a minor performance hit but is probably negligible and code simplicity is likely preferable. My suggestion would be to make the suggested change and remove the slice_new wrapper as well. Thank you!

esc

I'm helping out with the review in order for the PR to make it into 0.56 -- I reviewed all the comments and checked that all of @stuartarchibald 's comments have been addressed, except the comment about the core boxing implementation. I also left a comment regarding a potential test that could be added.

I think once these two changes have been made this is ready to merge. Thank you!!

PS: if you are running out of time, please do let me know, I'd be happy to volunteer to make the requested changes.

njriasan · 2022-06-17T15:04:37Z

@esc I think I have addressed all of the comments.

esc · 2022-06-20T14:28:36Z

@njriasan thank you for the changes. I have given the following two commits a quick performance test:

using 2b0a05943cc17bf1780a0e7f4a53d84dbe6ae592

In [2]: from numba import njit

In [3]: z = slice(1, 2, 3)

In [4]:         @njit
   ...:         def foo():
   ...:             return z
   ...:

In [5]: foo()
Out[5]: slice(1, 2, 3)

In [6]: %timeit foo()
74.3 ns ± 3.04 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

And with a476fa5c2abb4057ae27159f3ee8388db9922b3e:

In [1]: from numba import njit

In [2]:         @njit
   ...:         def foo():
   ...:             return z
   ...:

In [3]: z = slice(1, 2, 3)

In [4]: foo()
Out[4]: slice(1, 2, 3)

In [5]: %timeit foo()
529 ns ± 15.7 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

So that would indicate a performance hit -- but since this is in the nano-second range, I am not sure how reproducible these microbenchmarks are and how influential they will be.

njriasan · 2022-06-20T15:50:46Z

@esc I think since a slice won't ever scale beyond 3 elements and we are talking about nanoseconds, this is probably fine. However, I also don't consider the original implementation to be too complicated, so I would be fine with either decision.

njriasan · 2022-06-20T15:51:44Z

@esc I think since a slice won't ever scale beyond 3 elements and we are talking about nanoseconds, this is probably fine. However, I also don't consider the original implementation to be too complicated, so I would be fine with either decision.

I just saw your message on gitter. I agree the performance difference seems fine.

esc · 2022-06-20T16:24:48Z

@esc I think since a slice won't ever scale beyond 3 elements and we are talking about nanoseconds, this is probably fine. However, I also don't consider the original implementation to be too complicated, so I would be fine with either decision.

I just saw your message on gitter. I agree the performance difference seems fine.

OK, awesome, marking as Ready To Merge (RTM) now.

stuartarchibald · 2022-06-20T16:32:29Z

@njriasan many thanks for your work on this, @esc thanks for the review efforts.

esc · 2022-06-20T19:43:25Z

Thank you for the patch!

support for boxing SliceLiteral type

0294ef3

njriasan requested review from esc, sklam and stuartarchibald as code owners January 8, 2022 07:35

added decrefs for slice fields

06bc3bc

stuartarchibald added the 3 - Ready for Review label Jan 10, 2022

gmarkall added this to the Numba 0.56 RC milestone Jan 11, 2022

sklam assigned guilhermeleobas Jun 1, 2022

stuartarchibald unassigned guilhermeleobas Jun 1, 2022

guilhermeleobas approved these changes Jun 6, 2022

View reviewed changes

guilhermeleobas previously approved these changes Jun 6, 2022

View reviewed changes

stuartarchibald added Effort - medium Medium size effort needed 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jun 7, 2022

njriasan added 2 commits June 7, 2022 10:41

Merge branch 'master' into nick/literal_slice_boxing

29abf88

removed Type

4e2a179

njriasan dismissed guilhermeleobas’s stale review via 4e2a179 June 7, 2022 14:46

njriasan requested a review from guilhermeleobas June 7, 2022 14:47

stuartarchibald reviewed Jun 8, 2022

View reviewed changes

stuartarchibald self-assigned this Jun 8, 2022

njriasan added 3 commits June 13, 2022 22:56

Added docstring and fixed the int error message

1762ef2

fixed the docstring + added the extra test

8e2acaa

added stuart's TestCGContext test

2b0a059

sklam assigned esc and unassigned stuartarchibald Jun 14, 2022

esc requested changes Jun 16, 2022

View reviewed changes

njriasan added 3 commits June 17, 2022 01:28

added remaining test and fixed the error message

c7bcd41

added remaining test and fixed the error message

31b4a8e

Minor message changes

0a3502e

njriasan requested a review from esc June 17, 2022 05:30

Added val's requested tests

a476fa5

esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Jun 20, 2022

esc approved these changes Jun 20, 2022

View reviewed changes

esc merged commit c237ec5 into numba:main Jun 20, 2022

This was referenced Oct 21, 2025

Enable Boxing Unhashable Literals #7708

Closed

Cannot box Literals that aren't hashable #7707

Closed

Conversation

njriasan commented Jan 8, 2022

Uh oh!

guilhermeleobas left a comment

Choose a reason for hiding this comment

Uh oh!

stuartarchibald commented Jun 7, 2022

Uh oh!

njriasan commented Jun 7, 2022

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartarchibald Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

njriasan Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartarchibald commented Jun 8, 2022

Uh oh!

njriasan commented Jun 14, 2022

Uh oh!

esc commented Jun 16, 2022

Uh oh!

esc commented Jun 16, 2022

Uh oh!

esc left a comment

Choose a reason for hiding this comment

Uh oh!

njriasan commented Jun 17, 2022

Uh oh!

esc commented Jun 20, 2022

Uh oh!

njriasan commented Jun 20, 2022

Uh oh!

njriasan commented Jun 20, 2022

Uh oh!

esc commented Jun 20, 2022

Uh oh!

stuartarchibald commented Jun 20, 2022

Uh oh!

esc commented Jun 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

njriasan Jun 14, 2022 •

edited

Loading