On Friday 15 July 2011 21:52:09, Paul Pluzhnikov wrote: > Greetings, > > Following up on my earlier "slow on high-latency links" message: > http://sourceware.org/ml/gdb-patches/2011-07/msg00391.html ... > > Attached patch avoids calling solib_add twice when initially attaching > inferior. > > I am not entirely happy about this patch, but don't have a better idea > for a fix, and do want to avoid repeated rescans of the shared library list. Me neigher, apart from doing as the comment says: > /* Sometimes the platform-specific hook loads initial shared > libraries, and sometimes it doesn't. If it doesn't FROM_TTY will be > incorrectly 0 but such solib targets should be fixed anyway. If we > made all the inferior hook methods consistent, this call could be > removed. Call it only after the solib target has been initialized by > solib_create_inferior_hook. */ Or, making retrieving the dso list cheap enough to not care about the multiple calls. By e.g., caching more things, and making target_read_string not super dumb (as you've yourself proposed). Can you give a try on the attached patches and see how much a difference they make for you? We wrote these along with yesterday's tracepoint's one, exactly to speed up remote shared library loading/debugging on Android. Patch 2 has the most dramatic affect; patches 1 and 3 make some difference, though not as big. Luis wrote at the time: for patch 1: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This patch handles the caching of the AUXV block. Once loaded, GDB has access to the whole block of information. Queries can be made without the need to read the whole block from the remote target again). Some fields still require reading from the target (like the AT_PLATFORM field), but these tend to be smaller in size. The cache is invalidated whenever any of these events occur: inferior_exit, inferior_appeared and executable_changed. The data is kept in the following structure: struct auxv_info { LONGEST length; gdb_byte *data; }; The inferior_data machinery is used to keep per-inferior caches of the AUXV. Regressions: I regtested this patch and found no regressions on x86-32/64. Performance gains: This patch reduces the overhead a bit since read_program_header queries the AUXV machinery three times. There are other calls as well. Though the performance gains are small, they're still noticeable. for patch 2: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This patch (based on Pedro's patch) tries to optimize string reads from the target. Currently GDB reads 4 bytes at a time, and that's a big overhead if we're reading long path names. Experimenting with a few other block sizes, 64-bytes blocks seem to work great. This patch basically does the following: - Stabilish bigger block sizes for the string reads. - Replace target_read_memory with target_read_partial (Pedro's suggestion) to make sure we're testing for the terminating char at every tiny read (for targets that only support smaller packets). Worth mentioning that i did try using "hints" for the sizes, so we could adjust the block size to be close to the estimated length of the path names. This works nicely for long path names, but if we have a scenario where there's a pattern like short/long/short/long, i'm inclined to say it would perform just the same as using a standard 64-bytes block. Regressions: --- None. Regtested on x86-32/64 Performance gains: --- This is by far the patch that gives the most performance since it allows the reads to be performed in bigger blocks. This is specially interesting for remote targets, but also speeds up local targets a little. for patch 3: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This patch caches the program header information so we don't need to load it everytime. I've also used the inferior_data machinery to store such information. Additionaly, since it's a cache, i removed code that frees the allocated buffer after its use. That buffer is only deallocated inside the cleanup function instead of in functions that use this buffer. Regressions: --- Regtested the patch on x86-32/64 and found no additional errors. Performance gains: --- Reduces the overhead a little by caching the headers. I did some tests and noticed only a small improvement, but the function seems to be called a few times during the life of the process. It's worth just to reduce the number of unnecessary reads. > > (Some of our executables use 4000+ shared libraries, and the time in > solib_add does add up.) > > Tested on Linux/x86_64, no regressions. > > Comments? > > Thanks, > -- > Paul Pluzhnikov > > > > 2011-07-15 Paul Pluzhnikov > > * inferior.h (struct inferior): Add solib_add_generation. > * infcmd.c (post_create_inferior): Only call solib_add if not > already done. > * solib.c (solib_add): Increment solib_add_generation. > > > > Index: inferior.h > =================================================================== > RCS file: /cvs/src/src/gdb/inferior.h,v > retrieving revision 1.160 > diff -u -p -r1.160 inferior.h > --- inferior.h 3 Jun 2011 15:32:44 -0000 1.160 > +++ inferior.h 15 Jul 2011 20:10:20 -0000 > @@ -536,6 +536,9 @@ struct inferior > if any catching is necessary. */ > int total_syscalls_count; > > + /* This counts the number of solib_add() calls performed. */ > + int solib_add_generation; > + > /* Per inferior data-pointers required by other GDB modules. */ > void **data; > unsigned num_data; > Index: solib.c > =================================================================== > RCS file: /cvs/src/src/gdb/solib.c,v > retrieving revision 1.149 > diff -u -p -r1.149 solib.c > --- solib.c 30 Jun 2011 19:29:54 -0000 1.149 > +++ solib.c 15 Jul 2011 20:10:20 -0000 > @@ -914,6 +914,8 @@ solib_add (char *pattern, int from_tty, > { > struct so_list *gdb; > > + current_inferior ()->solib_add_generation++; > + > if (pattern) > { > char *re_err = re_comp (pattern); > Index: infcmd.c > =================================================================== > RCS file: /cvs/src/src/gdb/infcmd.c,v > retrieving revision 1.287 > diff -u -p -r1.287 infcmd.c > --- infcmd.c 30 May 2011 18:04:32 -0000 1.287 > +++ infcmd.c 15 Jul 2011 20:50:56 -0000 > @@ -398,6 +398,7 @@ void > post_create_inferior (struct target_ops *target, int from_tty) > { > volatile struct gdb_exception ex; > + int solib_add_generation; > > /* Be sure we own the terminal in case write operations are performed. */ > target_terminal_ours (); > @@ -419,6 +420,7 @@ post_create_inferior (struct target_ops > if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR) > throw_exception (ex); > > + solib_add_generation = current_inferior ()->solib_add_generation; > if (exec_bfd) > { > /* Create the hooks to handle shared library load and unload > @@ -432,14 +434,16 @@ post_create_inferior (struct target_ops > > /* If the solist is global across processes, there's no need to > refetch it here. */ > - if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch)) > + if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch) > + && current_inferior ()->solib_add_generation == solib_add_generation) > { > /* Sometimes the platform-specific hook loads initial shared > libraries, and sometimes it doesn't. If it doesn't FROM_TTY will be > incorrectly 0 but such solib targets should be fixed anyway. If we > made all the inferior hook methods consistent, this call could be > removed. Call it only after the solib target has been initialized by > - solib_create_inferior_hook. */ > + solib_create_inferior_hook. Only do this if not alreay done from > + inside solib_create_inferior_hook. */ > > #ifdef SOLIB_ADD > SOLIB_ADD (NULL, 0, target, auto_solib_add); > -- Pedro Alves