-
Notifications
You must be signed in to change notification settings - Fork 200
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
Introduced MovableBidirectionalIterator
#283
base: master
Are you sure you want to change the base?
Conversation
86629d6
to
88ce051
Compare
Just pushed a trivial unit test that's proof that it works as expected. I've also fixed formating by using tab, instead of space which makes generated code looks nice. |
c4c881d
to
5bfc90c
Compare
And some wording for the doc that makes it a bit cleaner. |
Realized that I've missed |
...and adding the same |
I hope that I've added it at all sorted Sets now. |
Added dummy implementation to empty sets |
@vigna any thoughts? |
Sorry, I somehow lost track of this. The problem I see is that it does not make any sense at the interface level—iterators do not necessarily come from sorted collections. |
yep, and I haven't found Anyway, it is quite useful at my case :) and I'll be very appricited if it can be merged. |
On 24 Mar 2023, at 14:03, Kirill A. Korinsky ***@***.***> wrote:
Anyway, it is quite useful at my case :) and I'll be very appricited if it can be merged.
I would probably merge in the implementations, but not the interface (as I said, it's not really sensible).
Ciao,
seba
|
I see your point and I agree with it, but I haven't found any nice way to inject that |
@vigna I'm willing to update this PR to make it easy to merge. To do it, may I ask some guidlines? Thanks! |
I really don't know. If this would be in Rust everything would be easier as we could combine traits. In the present situation I don't see alternatives to a JumpableIterator-or-something interface. I'm also not entirely convinced of the word "jump"—it suggest moving forward in the iterator sequence, but you're actually implementing a "move". In which case I'd probably have a "rewind()" method (move to the first item, like creating a new iterator) and a "from" method. That is, you can do after creation what you'd do at creation. Another problem is even if we go for the interface, there's no obvious way to have default implementations, and definitely I want to avoid other optional methods. Any thoughts? All this aside, do you have any evidence that in your case you have a measurable impact on performance? Small objects that are quickly used and discarded have almost zero impact with modern GCs. |
Let me start from the side note. I do have and this was made to make one services quite happy :) Long story short: it decreased response time about 2x in general. And for this services response time is one of critical things :) The usage of this feature is covered by micro benchmark, and here an example of JMH output with
and when I've replaced
As you may see impact into memory footprint is dramatical, and into speed is insignificant. So, as you had say it hasn't got anything in performance and mainly with GC. This micro benchmark is runs against faaaar less collections that a production one and production has impact more considerable to be honest, plus it affect in number and period of GC cycles... The services is running with Shenandoah as GC, I assume it modern enough :) and still less objects make things better and save heap for something else quit useful. Regarding your approach => I'll rename everything, and try to create a dedicated iterator, maybe it won't be as ugly as I though. Let see. |
@vigna do you prefer one more commit with rename and new iterator, or force push with rebased commit? |
MovableBidirectionalIterator
here it is. I've introduced It was cleaner that I've expected to be honest. |
Thinking about it. We're talking about bidirectional iterators. If we have rewind(), we need also something moving to the end. Or not? Just thinking. For example, if we're gonna implement those for linked hash maps, you could get to the start or end in constant time. Maybe |
I agree about moving to the end, but I was blocked because I can’t find the right naming :)
|
@vigna after some thoughts I agree that I've added support This is ready for review. |
On 17 Apr 2023, at 15:44, Kirill A. Korinsky ***@***.***> wrote:
And I do have one more "strange" idea on this iterator: mark() and reset() with behaviour similar to same function at InputStream.
What do you think about it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
It seems to put too much strain on implementors. No?
Ciao,
seba
|
@vigna yes, after a bit thinking of that mark-reset I've dismissed an idea and removed commit :) because it too wired |
@vigna any thought on that? |
Really on vacation now :). Ferragosto in Italy is sacred. Let's talk about this mid-September... |
@vigna how does vacation goes? :) |
This is a memory optimization for the case when a user required to modify iterator by moving it to a desire position. This works the same way as `iteraotr(fromElement)` but doesn't create a new iterator that decreases memory footprint at an algorithms which makes a lot of jumps. So, user simple calls `move(fromElement)`. It also introduces `begin()` which moves iterator to the first position, and `end()` which moves iterator to the last element.
5e1c9da
to
8af5777
Compare
@vigna just rebased it to the last master |
Ok, I know it's like forever but I'm looking into this. There should be no backward compatibility problem because you have default methods right? Just looking around. |
@vigna yes, no backward compatibility issues as far as I see. The default method which triggers an exception for this. |
What? |
@@ -24,5 +24,8 @@ public abstract class ABSTRACT_SORTED_SET KEY_GENERIC extends ABSTRACT_SET KEY_G | |||
protected ABSTRACT_SORTED_SET() {} | |||
|
|||
@Override | |||
public abstract KEY_BIDI_ITERATOR KEY_GENERIC iterator(); | |||
public abstract KEY_MOVE_BIDI_ITERATOR KEY_GENERIC iterator(KEY_GENERIC_TYPE fromElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now what happens to people who implemented a subclass of IntAbstractSortedSet but are returning a standard bidirectional iterator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I may move everything into KEY_BIDI_ITERATOR
if you think it is safe.
I mean that if someone call new method and hadn't got implemented it, an exception will be thrown. |
This is a memory optimization for the case when a user required to modify iterator by moving it to a desire position. This works the same way as
iteraotr(fromElement)
but doesn't create a new iterator that decreases memory footprint at an algorithms which makes a lot of jumps.