[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 Eric Gallager changed: What|Removed |Added Keywords||patch CC||egallager at gcc dot gnu.org See Also||https://gcc.gnu.org/bugzill ||a/show_bug.cgi?id=56746 --- Comment #19 from Eric Gallager --- (In reply to Richard Biener from comment #15) > (In reply to comment #14) > > Ping? Can someone review my last patch? I think it's clean enough to be > > applied (minus the TODO notes) and extra fixes can come separately later. > > Did you post it to gcc-patches? If so, ping there. Yup, it's in the URL field now: http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00284.html ...adding "patch" keyword.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #18 from Mathias Gaunard --- I'm not competent enough to make my own builds of GCC with patches, and I unfortunately do not have much time to contribute to this either. If someone can give me binaries for debian x86-64 I can do some tests though.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 Manuel López-Ibáñez changed: What|Removed |Added CC||manu at gcc dot gnu.org --- Comment #17 from Manuel López-Ibáñez --- (In reply to Mathias Gaunard from comment #16) > I opened bug #56746 a while ago which is somewhat related to this. > ftrack-macro-expansion is causing 50% increased memory usage in my case (C++ > code with heavy usage of macros and templates). > > Please also consider memory usage and not just performance. Perhaps you could try the patches posted here and report whether they reduce/increase memory usage? I am afraid that at the moment there is none with enough free time to work on this, so all the help we can get is appreciated. If you want me, i can close the other bug as a duplicate of this to have all info in a single bug, and update the description mentioning also memory usage.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 Mathias Gaunard changed: What|Removed |Added CC||mathias at gaunard dot com --- Comment #16 from Mathias Gaunard --- I opened bug #56746 a while ago which is somewhat related to this. ftrack-macro-expansion is causing 50% increased memory usage in my case (C++ code with heavy usage of macros and templates). Please also consider memory usage and not just performance.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #15 from Richard Guenther 2012-06-28 09:52:43 UTC --- (In reply to comment #14) > Ping? Can someone review my last patch? I think it's clean enough to be > applied > (minus the TODO notes) and extra fixes can come separately later. Did you post it to gcc-patches? If so, ping there.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #14 from jimis 2012-06-27 22:58:50 UTC --- Ping? Can someone review my last patch? I think it's clean enough to be applied (minus the TODO notes) and extra fixes can come separately later.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 jimis changed: What|Removed |Added Attachment #27520|0 |1 is obsolete|| Attachment #27523|0 |1 is obsolete|| --- Comment #13 from jimis 2012-06-04 04:49:13 UTC --- Created attachment 27550 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27550 Diff of all changes versus the 20120513 snapshot. I think I'm closing to a final version of this fix. The attached patch contains all of the above mentioned changes, plus it fixes the memory leaks. This bootstraps fine and passes tests on x86 with no regression. Instruction count has been reduced from 2201M downto 2108M, which is only 30M higher than having track-macro-expansion (TME) turned off. Unfortunately actual runtime was improved less, so we gained back almost 50% of what we had lost by enabling TME. In short running the same test as above, with this (macro5) patch, gives: 2108 M instr 31692 KB RAM 0.760s In a few words, I introduced four (!) new obstacks inside struct cpp_reader for allocating the tokens from macro argument expansion, their virtual locations, the virtual locations of arguments, and the virtual locations of other macros. I'm not sure whether this is an elegant solution but growing obstacks for nested scopes is much more intuitive than reallocating arrays. I'd appreciate any comments on my "TODO" notes in this patch, which mostly concern whether I should move other allocations to obstacks as well. Finally, my patch still contains the additions to obstacks (obstack_mark, obstack_release). I'll try submitting them to glibc but since they are in the obstack.h header file, they work even when using the libc obstacks so I guess they can be committed in the gcc tree.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #12 from jimis 2012-05-30 15:55:19 UTC --- I should probably explain where the problem is and why I've left a memory leak. In tokens_buff_new() I can't use XOBNEWVEC() instead of XNEWVEC() because it is not guarded from the obstack_mark/release() calls in enter_macro_context() and it messes up previously allocated objects when going back in the recursion. The thing I don't get and so I can't figure out a solution, is where the virt_locs vector allocated inside tokens_buff_new() is freed. For example when it is called from the path cpp_get_token_1->paste_all_tokens->tokens_buff_new(), then when is the virt locs vector actually freed?
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 Richard Guenther changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2012-05-30 Ever Confirmed|0 |1 --- Comment #11 from Richard Guenther 2012-05-30 08:38:15 UTC --- Confirmed.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #10 from jimis 2012-05-30 06:23:56 UTC --- Here is how this last patch (macro4) compares to trunk (TME) and to completely disabling track-macro-expansion (noTME): time M instr noTME 0.744s 2081 TME0.785s 2201 macro4 0.769s 2114
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #9 from jimis 2012-05-30 06:10:38 UTC --- Created attachment 27523 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27523 Move all location/expansion vectors to obstacks. Warning MEMLEAKS!
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525
--- Comment #8 from jimis 2012-05-30 06:06:16 UTC ---
Created attachment 27522
--> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27522
Introduce obstack_{mark,release} functions.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525
--- Comment #7 from jimis 2012-05-30 06:01:23 UTC ---
Now time for the most intrusive and problematic patches. I tried moving all
virt_locs, expanded, expanded_virt_locs to obstacks for allocation. After many
failures to work with obstacks as they are I had to introduce two new
functions, obstack_{mark,release}(), that allow me to continue growing an older
object by pushing and popping a special marker.
Previous patches are included with the following, but I'm submitting it
separately because it's WIP. Even though it is now stable, there is a bad
memory leak that I don't know how to handle since I'm kind of lost in the
macro-expansion code. I'd appreciate suggestions, just search for LEAK or TODO
in my comments. :-p
Please ignore whitespace changes (that are highlighted by emacs so I fix them)
or stylistic changes in parts that I rewrote so many times that lost completely
their initial style.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #6 from jimis 2012-05-30 05:31:03 UTC --- Created attachment 27521 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27521 Add some new obstack macros in libiberty.h.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #5 from jimis 2012-05-30 05:28:31 UTC --- Created attachment 27520 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27520 In macro.c:collect_args() use obstacks for virt_locs instead of malloc/realloc vectors.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #4 from jimis 2012-05-30 05:23:54 UTC --- Another hotspot higlighted by valgrind is the multitude of malloc/free() calls in comparison to the past. I'm attaching a slightly more intrusive patch that uses obstacks to allocate some of the virt_locs. It manages to save about 15 M instr or 2-3 ms, almost unnoticeable difference. Even though it seems to work fine it's not ready to commit, just an RFC for moving to obstacks. I'm also attaching some changes in libiberty.h that introduce some more XOB* macros.
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525
--- Comment #3 from jimis 2012-05-30 04:52:20 UTC ---
Another simple one that my eye caught but does not effect performance.
Generally I don't get many things in macro.c, but am I correct to assume that
the following stands?
=== modified file 'libcpp/macro.c'
--- libcpp/macro.c 2012-05-02 16:55:19 +
+++ libcpp/macro.c 2012-05-27 06:55:37 +
@@ -1897,10 +1897,9 @@ tokens_buff_new (cpp_reader *pfile, size
source_location **virt_locs)
{
size_t tokens_size = len * sizeof (cpp_token *);
- size_t locs_size = len * sizeof (source_location);
if (virt_locs != NULL)
-*virt_locs = XNEWVEC (source_location, locs_size);
+*virt_locs = XNEWVEC (source_location, len);
return _cpp_get_buff (pfile, tokens_size);
}
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 --- Comment #2 from jimis 2012-05-30 04:44:54 UTC --- According to valgrind major overhead is due to numerous calls of line-map.c:linemap_line_start() that actually allocate new line_maps. This happens because we are resetting the max_column_hint whenever we enter macro context in enter_macro_context()->linemap_enter_macro(). The following one-liner seems to resolve this, as run time is improved by about 7ms or 45 M Instr. === modified file 'libcpp/line-map.c' --- libcpp/line-map.c 2012-04-30 11:42:12 + +++ libcpp/line-map.c 2012-05-27 06:52:08 + @@ -331,7 +331,6 @@ linemap_enter_macro (struct line_maps *s num_tokens * sizeof (source_location)); LINEMAPS_MACRO_CACHE (set) = LINEMAPS_MACRO_USED (set) - 1; - set->max_column_hint = 0; return map; }
[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525 jimis changed: What|Removed |Added CC||abel at gcc dot gnu.org, ||amonakov at gcc dot ||gnu.org, dodji at gcc dot ||gnu.org, tromey at gcc dot ||gnu.org --- Comment #1 from jimis 2012-05-30 04:04:00 UTC --- I'll be posting some patches that try to improve CPU usage, some simple but most more complex that don't work properly so I'd appreciate advice. CC'ing mentors and tromey+Dodji who I think are the owners of the initial track-macro-expansion patch.
