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) > >> > > > > > >> > > > > >> > > > >> > > >> > > > > >
