add first element of empty queue to out list / return explicit empty queue #7873

gi114 · 2019-03-14T15:03:26Z

…ue if last element is dequeued

This is response to the request of optimization found here: scala/bug#11396

"immutable.Queue's methods should be optimised so that when dequeueing elements, it does not leave out empty (unless in is also empty), and when enqueueing elements, it puts at least one element in out if both in and out are empty (although once you're checking, might as well put everything in out)."

…ue if last element is dequeued

gi114 · 2019-03-19T13:31:15Z

Thanks, I signed the CLA !

NthPortal

tail (the method/part of the usage pattern listed in the linked ticket) also needs to be changed for this, as well as a bunch of other methods.

NthPortal · 2019-03-20T03:06:38Z

src/library/scala/collection/immutable/Queue.scala

    */
-  def enqueue[B >: A](elem: B): Queue[B] = new Queue(elem :: in, out)
+  def enqueue[B >: A](elem: B): Queue[B] =
+    if (isEmpty) new Queue(Nil, out :+ elem)


We generally don't want to append things (:+) to Lists - it is very slow. In this case however, we actually know the whole Queue is empty, so it can be elem :: Nil.

NthPortal · 2019-03-20T03:17:18Z

src/library/scala/collection/immutable/Queue.scala

  def dequeue: (A, Queue[A]) = out match {
-    case Nil if !in.isEmpty => val rev = in.reverse ; (rev.head, new Queue(Nil, rev.tail))
+    case Nil if !in.isEmpty => in match {
+      case x :: Nil => (x, new Queue(Nil, Nil))


I'm not sure how much this line adds. The cost of reversing a List with a single element is pretty low, and ideally doesn't happen very often (whereas this check has to be done for all reversals). Without benchmarks showing it's an improvement for near-empty Queues and not a significant cost for larger ones, I'm not convinced this check improves things.

Additionally, in order to fulfill the optimisation requested by the linked ticket, there should be a check for out having a single element, in which case in is eagerly reversed (slightly before it's needed, but presumably it's going to be removed eventually anyway - it's a queue after all).

(@gi114 is new to the repo, so might need pointers on running benchmarks if we decide that's needed here)

Thanks for your comments, I shall try to adjust the PR. And yes, this is my first ticket so let me know if I there is anything relevant I should do. Also, do you usually run tests suites or do you test your changes separately on a .gitignore file?

gi114 · 2019-03-22T13:42:23Z

Hi, I updated my PR with some other changes as it is shown above

Rationale behind changes is the following: The fist element added to an empty queue is added to 'out', as specified by the ticket, and 'out' should never be forced empty. Because of that, dequeuing elements involves 'in' eagerly reversing before the 'out' list is ever empty ( one step before, so when 'out' has only one element ).
Thus, the .tail method matches the following conditions: if the 'in' list is non empty for the first case, for the second case it matches single or multiple elements in 'out' (which is never empty), and the third case is the exception of calling tail on an empty queue. In this way we never encounter an empty 'out' unless the Queue is emptied. We should make sure all functions under immutable Queue operate on this constrain, so let me know if you find that it is not the case. Very importantly, with this constrain we would not need to worry to call .head on some empty 'out' list and and traverse 'in' with O(n) to find the last element, so the return value is always out.head, unless the queue is empty

stale

…ase of empty queue, the first iterable input is added to out. This is in line with previous commits on enqueue and dequeue methods

gi114 · 2019-04-08T14:40:42Z

Fixed a test which was failing. Everything seems fine, the PR follows a specific logic. Let me know if agree or see anything wrong.

Also, as am new, I wanted to ask you if you could point at how to run some code performance analysis. Thanks again

NthPortal

This looks great and takes care of all the obvious methods.

However, I am concerned that there may be some other method that doesn't comply with the new invariant "if out is empty then in must also be empty." These implementations currently assume/require the invariant to hold, and if it does not, they will do the wrong thing.

It would certainly be a good idea to have someone run collection-laws on this; additionally, I will attempt to comb through all the other methods to check that they maintain the invariant.

I think it would also be good to add a comment somewhere (I'm not sure exactly where, tbh) stating the invariant that all methods must maintain.

src/library/scala/collection/immutable/Queue.scala

if it's empty, in and out are both Nil Co-Authored-By: Nth <7505383+NthPortal@users.noreply.github.com>

gi114 · 2019-05-31T14:07:14Z

Hi,

Would you think this pull request is ready to be merged ?

Thanks!

NthPortal · 2019-05-31T15:13:42Z

Unfortunately, this cannot be merged until 2.13.0 is released, as it is not a blocker for the release. Once it is and we move on to 2.13.1 however, we will be able to look at this PR again

gi114 · 2019-05-31T18:07:45Z

Ok, I see. Thanks a lot for your clarification Il giorno ven 31 mag 2019 alle ore 11:14 Nth <notifications@github.com> ha scritto:

…

Unfortunately, this cannot be merged until 2.13.0 is released, as it is not a blocker for the release. Once it is and we move on to 2.13.1 however, we will be able to look at this PR again — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7873?email_source=notifications&email_token=AEGS2Z4BWIHQJINTHDA5XGTPYE6EPA5CNFSM4G6RQFW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWVP7ZA#issuecomment-497745892>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGS2Z2H4BZREFDX5POGX7TPYE6EPANCNFSM4G6RQFWQ> .

NthPortal · 2019-05-31T21:59:06Z

@gi114 sorry for the wait, but we'll get there :)

szeiger · 2019-08-20T13:35:41Z

Any progress on this? Feel free to reschedule to 2.13.2 if it will take longer.

NthPortal · 2019-08-21T01:13:04Z

I'll try to put it on my weekend TODO list

gi114 · 2019-08-22T01:44:46Z

Hi, let me know if there is anything I can add/change from a code perspective on this one. Would you be able to rerun the tests to see if anything is failing? Thanks and let me know!

NthPortal · 2019-09-28T08:40:35Z

I'm so sorry that I can't get to this right now. if someone else would like to take over getting this over the finish line, that would be great

gi114 · 2019-09-29T16:11:10Z

Hi, I will be happy to try. Would you let me know if there is any test I should perform on this one? Thanks Giulia Il giorno sab 28 set 2019 alle ore 04:41 NthPortal <notifications@github.com> ha scritto:

…

I'm so sorry that I can't get to this right now. if someone else would like to take over getting this over the finish line, that would be great — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7873?email_source=notifications&email_token=AEGS2Z4NBG6QTOQTBGOG7YDQL4KCDA5CNFSM4G6RQFW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD72UCVA#issuecomment-536166740>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGS2ZYJLGMUOPB6WZKBCTDQL4KCDANCNFSM4G6RQFWQ> .

som-snytt · 2019-10-01T06:51:03Z

src/library/scala/collection/immutable/Queue.scala

-  def enqueue[B >: A](elem: B): Queue[B] = new Queue(elem :: in, out)
+  def enqueue[B >: A](elem: B): Queue[B] =
+    if (isEmpty) new Queue(Nil, elem :: Nil)
+    else new Queue(elem :: in, out)


This seems like an easy win. I wonder if there is a good way to test it. Maybe a benchmark is the only test that really matters. If a test that just continually enqueues and dequeues a single element at a time doesn't get a bit faster, would anything?

som-snytt · 2019-10-01T06:55:42Z

src/library/scala/collection/immutable/Queue.scala

  override def head: A =
-    if (out.nonEmpty) out.head
-    else if (in.nonEmpty) in.last
+    if (nonEmpty) out.head


I'm dubious, that is, I think this is dubious.

som-snytt · 2019-10-01T07:02:12Z

src/library/scala/collection/immutable/Queue.scala

    */
  def dequeue: (A, Queue[A]) = out match {
-    case Nil if !in.isEmpty => val rev = in.reverse ; (rev.head, new Queue(Nil, rev.tail))
+    case x :: Nil if in.nonEmpty => (x, new Queue(Nil, in.reverse))


I'm not sure incurring the reverse eagerly is worth the goal of supporting dubious head/tail pattern described in the ticket. As an aside, there is ongoing conversation about whether to always prefer !isEmpty to nonEmpty for performance reasons (because cost is not zero).

Queue is a LinearSeq so there is an expectation that head/tail should have decent performance.

I didn't look at the generated code but going by the List implementation I would definitely prefer to call isEmpty when the type is known (like it is in this case). The Scala optimizer should be able to inline the method so you don't even have to rely on HotSpot.

som-snytt · 2019-10-01T07:11:50Z

I don't know if encouraging the head/tail anti-pattern is desirable, but I'll comment about that on the ticket.

I did spot a potential improvement at #8449 which I guess is desirable, but it is late here. I touched the junit test for this class there.

Do you know the local customs to squash commits and push -f to this branch and run sbt junit/test?

I see the previous question whether it's easy to show that out.nonEmpty always holds. (I prefer nonEmpty, of course.) I would suggest that if it's not easy to show that much, then it's not worth any complexity.

However, always putting the first element in out seems good to me.

gi114 · 2019-10-01T12:29:44Z

Hi, Would you point me at the resources you used for writing benchmarks and other examples. It is a new topic for me but would be useful to see the performance improvement, if any Thanks Il giorno mar 1 ott 2019 alle ore 03:12 som-snytt <notifications@github.com> ha scritto:

…

I don't know if encouraging the head/tail anti-pattern is desirable, but I'll comment about that on the ticket. I did spot a potential improvement at #8449 <#8449> which I guess is desirable, but it is late here. I touched the junit test for this class there. Do you know how to squash commits and run sbt junit/test? I see the previous question whether it's easy to show that out.nonEmpty always holds. (I prefer nonEmpty, of course.) I would suggest that if it's not easy to show that much, then it's not worth any complexity. However, always putting the first element in out seems good to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7873?email_source=notifications&email_token=AEGS2Z3LFBD55LUZMZU52Y3QMLZ6FA5CNFSM4G6RQFW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAAHSCI#issuecomment-536901897>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGS2Z2AJGUUU4SSCB77CN3QMLZ6FANCNFSM4G6RQFWQ> .

NthPortal · 2019-12-01T00:03:00Z

anyone else have thoughts on @som-snytt's comment about whether or not this is a desirable change?

szeiger · 2019-12-09T16:45:58Z

@gi114 See https://github.com/scala/scala/blob/2.13.x/test/benchmarks/README.md for benchmarking. There's already a number of benchmarks for other collections that would make a good starting point. One thing the readme doesn't mention (but really should) is that you need to turn on optimization for the standard library (e.g. setupPublishCore) to get useful results. The JMH documentation has more information about how to write good benchmarks.

szeiger · 2020-04-01T16:59:03Z

This still looks like a worthwhile improvement to me. There were some changes in the 2.13.x implementation in the meantime so this would need to be rebased. In particular, the code for enqueueAll is now in appendedAll.

As long as enqueue, dequeue, tail and appendedAll are modified to keep the new invariants, everything else should work fine. I think head could be simplified further to only check out, which is cheaper than both the current implementation and the one proposed here.

There should still be some basic benchmarks to ensure we're not regressing in more normal use cases though.

SethTisue · 2020-09-30T19:15:07Z

is anyone interested in picking this one up?

NthPortal · 2020-10-01T15:00:51Z

I would like to, but I don't have the capacity right now. if no one picks it up in a month, I'll see if I have the capacity for it

dwijnand · 2020-12-04T16:36:46Z

Closing for inactivity. Happy to reopen any time, by Nth or anyone else.

add first element of empty queue to out list/return explicit emty que…

278fd63

…ue if last element is dequeued

scala-jenkins added this to the 2.13.1 milestone Mar 14, 2019

diesalbla added the library:collections PRs involving changes to the standard collection library label Mar 17, 2019

SethTisue requested a review from NthPortal March 19, 2019 18:55

NthPortal previously requested changes Mar 20, 2019

View reviewed changes

SethTisue added the welcome hello new contributor! label Mar 20, 2019

Out list never left empty in immutable Queue

6bf0608

gi114 mentioned this pull request Mar 29, 2019

Optimise immutable.Queue for head/tail scala/bug#11396

Open

After immutable QueueTest failure I modified enqueueAll so that, in c…

96e0af5

…ase of empty queue, the first iterable input is added to out. This is in line with previous commits on enqueue and dequeue methods

NthPortal self-assigned this Apr 21, 2019

NthPortal reviewed Apr 21, 2019

View reviewed changes

src/library/scala/collection/immutable/Queue.scala Outdated Show resolved Hide resolved

Update src/library/scala/collection/immutable/Queue.scala

3427bcd

if it's empty, in and out are both Nil Co-Authored-By: Nth <7505383+NthPortal@users.noreply.github.com>

szeiger modified the milestones: 2.13.1, 2.13.2 Aug 30, 2019

NthPortal removed their assignment Sep 28, 2019

som-snytt reviewed Oct 1, 2019

View reviewed changes

NthPortal closed this Dec 1, 2019

NthPortal reopened this Dec 1, 2019

SethTisue modified the milestones: 2.13.2, 2.13.3 Feb 6, 2020

SethTisue modified the milestones: 2.13.3, 2.13.4 May 12, 2020

SethTisue marked this pull request as draft May 19, 2020 01:05

dwijnand modified the milestones: 2.13.4, 2.13.5 Oct 16, 2020

dwijnand closed this Dec 4, 2020

SethTisue changed the title ~~add first element of empty queue to out list / return explicit emty que…~~ add first element of empty queue to out list / return explicit empty queue Feb 9, 2021

SethTisue removed this from the 2.13.5 milestone Feb 9, 2021

add first element of empty queue to out list / return explicit empty queue #7873

add first element of empty queue to out list / return explicit empty queue #7873

Uh oh!

Conversation

gi114 commented Mar 14, 2019

Uh oh!

gi114 commented Mar 19, 2019

Uh oh!

NthPortal left a comment

Choose a reason for hiding this comment

Uh oh!

NthPortal Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

NthPortal Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

SethTisue Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

gi114 Mar 21, 2019 • 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!

gi114 commented Mar 22, 2019

Uh oh!

gi114 commented Apr 8, 2019

Uh oh!

NthPortal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gi114 commented May 31, 2019

Uh oh!

NthPortal commented May 31, 2019

Uh oh!

gi114 commented May 31, 2019 via email

Uh oh!

NthPortal commented May 31, 2019

Uh oh!

szeiger commented Aug 20, 2019

Uh oh!

NthPortal commented Aug 21, 2019

Uh oh!

gi114 commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NthPortal commented Sep 28, 2019

Uh oh!

gi114 commented Sep 29, 2019 via email

Uh oh!

som-snytt Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

som-snytt Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

som-snytt Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

szeiger Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gi114 commented Oct 1, 2019 via email

Uh oh!

NthPortal commented Dec 1, 2019

Uh oh!

szeiger commented Dec 9, 2019

Uh oh!

szeiger commented Apr 1, 2020

Uh oh!

SethTisue commented Sep 30, 2020

Uh oh!

NthPortal commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwijnand commented Dec 4, 2020

gi114 Mar 21, 2019 •

edited

Loading

gi114 commented Aug 22, 2019 •

edited

Loading

som-snytt commented Oct 1, 2019 •

edited

Loading

NthPortal commented Oct 1, 2020 •

edited

Loading