[Crash-utility] Faster iteration on list of struct.field

[Dave Anderson]

----- Original Message -----
> Dave Anderson wrote on Thu, Feb 06, 2020:
> > Right, the time-consumer is the call into gdb to get the structure member
> > details.
> > 
> > I'm not following what you mean by "making 'datatype_member' static ...",
> > but
> > off the top of my head, I was thinking of adding a "tmp_offset" and
> > "tmp_size"
> > fields to the global offset_table and size_table structures, and adding a
> > new
> > pc->curcmd_flags bit.  Then, if a command that wants to support such a
> > concept,
> > it could:
> > 
> >   (1) check the new pc->curcmd_flags bit, which will always be 0 the first
> >   time
> >       the function gets called by the exec_args_input_file() loop.
> >   (2) if the new bit is 0, then set OFFSET(tmp_offset) and SIZE(tmp_size)
> >   (3) set the new flag in pc->curcmd_flags, and continue...
> > 
> > Then during the second and subsequent times through the loop,
> > pc->curcmd_flags
> > will still be set/unchanged, because restore_sanity() is not called from
> > the
> > exec_args_input_flags() loop.
> > 
> > But that scheme falls down if a user requests a comma-separated list of
> > multiple members (argc_members would be > 1).  Although, it might be better
> > if the "struct -r' option rejects multiple-member arguments, especially
> > given
> > that the output would be pretty much unreadable.
> 
> I would tend to agree with that, struct -r with multiple members might
> be somewhat parsable but if someone can do that they can dump the whole
> struct and parse it anyway, so let's go with only one number.
> 
> On the good news though, this whole caching isn't going to be
> immediately needed. I just finished the first part of this (allowing
> struct -r with a member), and struct -r is already infinitely faster
> than struct; so getting the offset wasn't the slow part:
>  - with a small 100-elements file, I'm already going down from 12s to
> near-instant on this old laptop.
>  - I didn't wait for 1000-elements to finish normally but it's just
> about one second with -r, which is acceptable enough for me.
> 
> Caching might make it another order of magnitude faster but for now I'm
> happy to wait a couple of minutes for my 100k elements list, it's better
> than not finishing in half an hour :)

Ok, so it must be the gdb-assisted print_struct() and parsing that's 
time-consuming, and not the gdb datatype query.

> 
> Following up with patch, with a couple of remarks:
>  - I had to change member_to_datatype() to use datatype_info() directly
> instead of MEMBER_OFFSET(), to fill dm->member_size. I'm not sure if
> this will have any side effects, but things like 'struct foo.a,b' still
> work at least. You might have a better idea of what to check.

Hmmm, I'd prefer to keep the member_datatype() behavior as it is, and
not have datatype_info() re-initialize the incoming dm.  (except for
the setting of dm->member).  Maybe have a different flag for gathering
the size as you have, and keep the original functionality the same?  
Or alternatively, leave the call to member_datatype() as-is, and if 
do_datatype_addr() sees SHOW_RAW_DATA, additionally call MEMBER_SIZE()?

>  - I'm only passing ANON_MEMBER_QUERY to member_to_datatype() in the
> non-raw case. 

I think you mean just the opposite...

                               if (!member_to_datatype(memberlist[i], dm,
-                                                       ANON_MEMBER_QUERY))
+                                                       (flags & SHOW_RAW_DATA) ? ANON_MEMBER_QUERY : 0))
                                        error(FATAL, "invalid data structure reference: %s.%s\n",
                                              dm->name, memberlist[i]);

The use of ANON_MEMBER_QUERY is just there for a fall-back option if the
MEMBER_OFFSET() call fails.  

>                I'm not quite sure why we couldn't get the member size if
> it's an anon union/stuct, but I'm not sure how one would name an
> anonymous field in the first place here? Anyway, one would get invalid
> data structure reference message there if they do. It might be better
> to always pass the argument and then check for SHOW_RAW_DATA set with
> dm->member_size still being 0 after call to give another more
> appropriate error if you think people might hit that.

All of the ANON_xxx macros were added for getting information for members
that are declared inside of anonymous structures within a structure, where
where the generic datatype_info() call fails.  In those cases, the request
gets directed to a gdb print command within anon_member_offset().  There is
no support for getting the size of such a member, so MEMBER_SIZE() would
fail.  So I don't think this feature would work for those types of members,
and would need some kind of ANON_MEMBER_SIZE() and accompanying 
anon_member_size() functionality.

Dave

> Anyway, thanks for the advices.
> --
> Dominique
> 
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>