I am working on a patch. Here is a sample. This one is for RASV. I put
Dan's example as a test case.
1. private final class NonDefaultIterator implements Iterator<Element>
{
2. private final class NonDefaultElement implements Element {
3. @Override
4. public double get() {
5. return mapElement.get();
6. }
7.
8. @Override
9. public int index() {
10. return mapElement.index();
11. }
12.
13. @Override
14. public void set(double value) {
15. invalidateCachedLength();
16. mapElement.set(value);
17. }
18. }
19.
20. private final NonDefaultElement element = newNonDefaultElement();
21. private final Iterator<MapElement> iterator;
22. private MapElement mapElement;
23.
24. private NonDefaultIterator() {
25. this.iterator = values.iterator();
26. }
27.
28. @Override
29. public boolean hasNext() {
30. return iterator.hasNext();
31. }
32.
33. @Override
34. public Element next() {
35. mapElement = iterator.next();
36. return element;
37. }
38.
39. @Override
40. public void remove() {
41. throw new UnsupportedOperationException();
42. }
43. }
44.
Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc.
On Sun, Apr 14, 2013 at 7:04 PM, Ted Dunning <[email protected]> wrote:
> Well... current iterator style with a non-side-effecting version of
> hasNext(), of course.
>
> Reusing the container is OK if the performance hit is substantial.
>
>
> On Sun, Apr 14, 2013 at 5:02 PM, Robin Anil <[email protected]> wrote:
>
> > Also the Tests crash due to excessive GC. The performance degradation
> there
> > is very visible(my cpu spikes up). I think there is good case for the
> > current iteration style, just that we have to, not use the java Iterator
> > contract and confuse clients.
> >
> > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc.
> >
> >
> > On Sun, Apr 14, 2013 at 6:59 PM, Robin Anil <[email protected]>
> wrote:
> >
> > > Yes. All final.
> > >
> > > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc.
> > >
> > >
> > > On Sun, Apr 14, 2013 at 6:55 PM, Ted Dunning <[email protected]
> > >wrote:
> > >
> > >> Did you mark the class and fields all as final?
> > >>
> > >> That might help the compiler realize it could in-line stuff and avoid
> > the
> > >> constructor (not likely, but possible)
> > >>
> > >>
> > >> On Sun, Apr 14, 2013 at 4:52 PM, Robin Anil <[email protected]>
> > wrote:
> > >>
> > >> > With a new immutable Element in the iterator, the iteration behavior
> > is
> > >> > corrected but. There is a performance degradation of about 10% and
> > >> > nullifies what I have done with the patch.
> > >> >
> > >> > See
> > >> >
> > >> >
> > >>
> >
> https://docs.google.com/spreadsheet/ccc?key=0AhewTD_ZgznddGFQbWJCQTZXSnFULUYzdURfWDRJQlE#gid=1
> > >> >
> > >> > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc.
> > >> >
> > >> >
> > >> > On Sun, Apr 14, 2013 at 11:28 AM, Ted Dunning <
> [email protected]>
> > >> > wrote:
> > >> >
> > >> > > Yeah... but we still have to fix the iterator.
> > >> > >
> > >> > >
> > >> > > On Sun, Apr 14, 2013 at 8:58 AM, Robin Anil <[email protected]
> >
> > >> > wrote:
> > >> > >
> > >> > > > Here is an iteration style that works as is with today's
> behaviour
> > >> of
> > >> > > > hasNext
> > >> > > >
> > >> > > > 1.
> > >> > > > 2. Element thisElement = null;
> > >> > > > 3. Element thatElement = null;
> > >> > > > 4. boolean advanceThis = true;
> > >> > > > 5. boolean advanceThat = true;
> > >> > > > 6.
> > >> > > > 7. Iterator<Element> thisNonZero =
> this.iterateNonZero();
> > >> > > > 8. Iterator<Element> thatNonZero = x.iterateNonZero();
> > >> > > > 9.
> > >> > > > 10. double result = 0.0;
> > >> > > > 11. while (true) {
> > >> > > > 12. *if (advanceThis) {
> > >> > > > *
> > >> > > > 13. * if (!thisNonZero.hasNext()) {
> > >> > > > *
> > >> > > > 14. * break;
> > >> > > > *
> > >> > > > 15. * }
> > >> > > > *
> > >> > > > 16. * thisElement = thisNonZero.next();
> > >> > > > *
> > >> > > > 17. * }
> > >> > > > *
> > >> > > > 18. * if (advanceThat) {
> > >> > > > *
> > >> > > > 19. * if (!thatNonZero.hasNext()) {
> > >> > > > *
> > >> > > > 20. * break;
> > >> > > > *
> > >> > > > 21. * }
> > >> > > > *
> > >> > > > 22. * thatElement = thatNonZero.next();
> > >> > > > *
> > >> > > > 23. * }*
> > >> > > > 24. if (thisElement.index() == thatElement.index()) {
> > >> > > > 25.
> > >> > > > 26. result += thisElement.get() *
> thatElement.get();
> > >> > > > 27. advanceThis = true;
> > >> > > > 28. advanceThat = true;
> > >> > > > 29. } else if (thisElement.index() <
> > >> thatElement.index()) {
> > >> > > > 30. advanceThis = true;
> > >> > > > 31. advanceThat = false;
> > >> > > > 32. } else {
> > >> > > > 33. advanceThis = false;
> > >> > > > 34. advanceThat = true;
> > >> > > > 35. }
> > >> > > > 36. }
> > >> > > >
> > >> > > >
> > >> > > > On Sat, Apr 13, 2013 at 1:47 AM, Ted Dunning <
> > [email protected]
> > >> >
> > >> > > > wrote:
> > >> > > >
> > >> > > > > The caller is not at fault here. The problem is that hasNext
> is
> > >> > > > advancing
> > >> > > > > the iterator due to a side effect. The side effect is
> > impossible
> > >> to
> > >> > > > avoid
> > >> > > > > at the level of the caller.
> > >> > > > >
> > >> > > > > Sent from my iPhone
> > >> > > > >
> > >> > > > > On Apr 12, 2013, at 12:22, Sean Owen <[email protected]>
> wrote:
> > >> > > > >
> > >> > > > > > I'm sure I did (at least much of) the AbstractIterator
> change
> > so
> > >> > > blame
> > >> > > > > > me... but I think the pattern itself is just fine. It's used
> > in
> > >> > many
> > >> > > > > > places in the project. Reusing the value object is a big win
> > in
> > >> > some
> > >> > > > > > places. Allocating objects is fast but a trillion of them
> > still
> > >> > adds
> > >> > > > > > up.
> > >> > > > > >
> > >> > > > > > It does contain a requirement, and that is that the caller
> is
> > >> > > supposed
> > >> > > > > > to copy/clone the value if it will be used at all after the
> > next
> > >> > > > > > iterator operation. That's the 0th option, to just fix the
> > >> caller
> > >> > > > > > here.
> > >> > > > > >
> > >> > > > > > On Fri, Apr 12, 2013 at 7:49 PM, Ted Dunning <
> > >> > [email protected]>
> > >> > > > > wrote:
> > >> > > > > >> The contract of computeNext is that there are no side
> effects
> > >> > > visible
> > >> > > > > >> outside (i.e. apparent functional style). This is required
> > >> since
> > >> > > > > >> computeNext is called from hasNext().
> > >> > > > > >>
> > >> > > > > >> We are using a side-effecting style so we have a bug.
> > >> > > > > >>
> > >> > > > > >> We have two choices:
> > >> > > > > >>
> > >> > > > > >> a) use functional style. This will *require* that we
> > allocate a
> > >> > new
> > >> > > > > >> container element on every call to computeNext. This is
> best
> > >> for
> > >> > > the
> > >> > > > > user
> > >> > > > > >> because they will have fewer surprising bugs due to reuse.
> > If
> > >> > > > > allocation
> > >> > > > > >> is actually as bad as some people think (I remain skeptical
> > of
> > >> > that
> > >> > > > > without
> > >> > > > > >> tests) then this is a bad move. If allocation of totally
> > >> > ephemeral
> > >> > > > > objects
> > >> > > > > >> is as cheap as I think, then this would be a good move.
> > >> > > > > >>
> > >> > > > > >> b) stop using AbstractIterator and continue with the re-use
> > >> style.
> > >> > > > And
> > >> > > > > add
> > >> > > > > >> a comment to prevent a bright spark from reverting this
> > change.
> > >> > (I
> > >> > > > > suspect
> > >> > > > > >> that the bright spark who did this in the first place was
> me
> > >> so I
> > >> > > can
> > >> > > > be
> > >> > > > > >> rude)
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>
