On 30/3/21 9:10 am, Alex White wrote: > On Mon, Mar 29, 2021 at 4:39 PM Chris Johns <chr...@rtems.org> wrote: >> >> On 30/3/21 7:56 am, Gedare Bloom wrote: >>> On Fri, Mar 12, 2021 at 10:07 AM Alex White <alex.wh...@oarcorp.com> wrote: >>>> >>>> This adds a new behavior to the -d option which allows the creation of >>>> named objdump outputs in the /tmp directory. This allows the outputs to >>>> be reused on subsequent runs of covoar rather than running objdump >>>> again. >>> This seems to be a hackish way to optimize a behavior. Do you guys >>> have a plan to improve this caching mechanism? >> >> Agreed. If objdump and generated files is being used then caching and >> management >> across instances of covoar will be complex but that is one of the trade-offs >> when wrapping objdump. > > At the moment, there is no plan to improve it, no. > > The original behavior of -d was just to keep the temp files around after a > run. > This change augments that behavior to also give the temp files useful names. > It > allows for a nice speedup when trying to do multiple runs for debugging > purposes since the temp files can now be checked to see if they need to be > regenerated.
This is different to improving performance and that is what the commit comments say. And changing the commit comments now to discuss a debug option that is there for performance testing only makes life harder for us as reviewers. > Ideally, yes, this would be more sophisticated and less hackish. I hid it > behind > the debug flag since it was added as a means to test my changes more quickly > rather than as a fully-fledged file caching feature. Great and so it seems the test has worked. There is no need to merge the test code. A full solution would be welcome. > If the caching mechanism were improved, I would likely move it to its own > flag. > That would allow the speedup to be realized on more than just debug runs. Great, sounds like a plan. >>>> - throw rld::error( "Objdump error", "generating objdump" ); >>>> + if (!debug || FileIsNewer( fileName.c_str(), >>>> objdumpFile.name().c_str() )) { >>> what's this !debug > > `debug` is set at the top level to `true` if the "-d" option is provided, > false > otherwise. Yes I know. I changed that top level to be more C++ 3 years ago unfunded to try and provide a pathway for more changes to happen. >>> Relying on timestamps can be tricky. You all should consider how to >>> create a proper cache of files, probably in the local directory from >>> which this tool is run, that updates based on hashed contents of the >>> source files rather than timestamps. This is for your TODO list. >> >> The commit comment says the files are in /tmp. Using /tmp to hold files that >> are >> not temporary seems the contradict the idea of a temporary directory. >> >> Who cleans up the files in /tmp that are left there? > > The original behavior of the "-d" option was to preserve the generated files > in the /tmp directory, I believe. This is the part that is disconnected and does not match. I am concerned it becomes a back door performance hack. Some systems use RAM disks for /tmp for performance so having it fill would be a problem. > This behavior is unchanged. I think it is changed with this patch. >>>> +{ >>>> + size_t idx = path.rfind('/', path.length()); >>> Are there any library helpers that can do this? > > None that I have found. Hmmmm .... https://git.rtems.org/rtems-tools/tree/rtemstoolkit/rld-path.h#n44 This is not the first tool in our tool suite that has had to deal with this. >>> >>> What happens if you run this on windows? (I know the answer, this >>> stuff doesn't work on windows. But it doesn't mean we shouldn't >>> consider cross-platform issues?) >> >> Windows is a supported host for this project so I hope it is tested on >> Windows. >> Not building or not working just means someone else needs to fix the problem. > > I was only running with `-d` on Linux and FreeBSD so I wasn't thinking about > it. > > Should I change it to handle separators on Windows using an ifdef? No. I suggest to stop guessing and testing our patience. Chris _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel