Community
Participate
Working Groups
We should investigate how to optimize the generation of line numbers using the new method getLineNumber(..) using a better scope for searching.
Created attachment 67566 [details] Proposed fix This improves slightly the performance. Frédéric will confirm the new perfs results.
Created attachment 68161 [details] Proposed fix This patch seems to make a 1.5% improvement in some performance tests.
Jérôme, Frédéric, please review.
I definitely think I lack knowledge on code generation to be really efficient while reviewing this big - and I guess risky - change... Can you ask someone who already has this background to review it? Thanks
Maxime, Philippe, please review.
Olivier, I'm afraid a few things look odd to me, but this may come from a lack of familiarity with that code area: - on line 5784, I don't see how we could have length equal to 0; if it were so, we would have no line separator, hence the method would be on a single line and we would have executed line 5778 instead? - I am not sure why you are passing this.lineNumberEnd - 1 to getLineNumber; is that to save a loop iteration? if so, this would deserve a comment. I've not dug deeper so far, since I'd like to better understand the overall picture. Other remarks: - I would not initialize lineNumber when declaring it; this will let the compiler tell you if all branches initialize it properly (and 0 is not a valid value anyway - we save an assigment); - while this is not related to your patch, I found Util#getLineNumber especially difficult to read and to understand; having it specified could help sort out what is expected, especially the parameters domains; I've not proved it formally, but the use of while (g <= d) might be replaced by a while (g < d), saving an iteration and a noop assignment in some cases, provided that the case when position matches a line separator be handled properly.
(In reply to comment #6) > Olivier, I'm afraid a few things look odd to me, but this may come from a lack > of familiarity with that code area: > - on line 5784, I don't see how we could have length equal to 0; if it were so, > we would have no line separator, hence the method would be on a single line and > we would have executed line 5778 instead? > - I am not sure why you are passing this.lineNumberEnd - 1 to getLineNumber; is > that to save a loop iteration? if so, this would deserve a comment. > I've not dug deeper so far, since I'd like to better understand the overall > picture. I'll provide a new patch for these issues.
Created attachment 68398 [details] Proposed fix Since with the fix for bug 188656, we are back to M7 perfs, I would move this one to 3.3.1 instead of 3.3RC2. This would give us more time to test this patch.
If we no more *need* it for 3.3, I vote +1 for deferring to 3.3.1. My first reading gave me the feeling that the fix was far from being trivial. I'd also support some reconsideration of Util#getLineNumber, and maybe some white box testing as well (this is not the norm in the team since we prefer user level test cases by any measure, but a nicely separated complex algorithm is especially suited to white box testing, IMHO).
I am fine to defer until 3.3.1.
Moving to 3.3.1.
Created attachment 74989 [details] Update previous patch for HEAD contents Resynch the previous patch with actual HEAD contents.
Thinking again, why are we considering a minor perf improvement for a maintenance release ? Feels like asking for trouble, and thus this should go in 3.4 only.
Released for 3.4M2. To verify, an improvement should be noticeable in the performance tests.
Verified for 3.4M2
