This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] Preserve order of dlopen'd modules in dll_list::topsort


Hi Corinna,

Thanks for the feedback!

Corinna Vinschen wrote:
> Hi David,
> 
> thanks for the new patch.
> 
> On Feb 27 17:13, David Allsopp wrote:
> > Corinna Vinschen wrote:
> > > On Feb 25 16:27, David Allsopp wrote:
> > > > This patch (below - I hope I have managed to format this email
> > > > correctly) alters the behaviour of dll_list::topsort to preserve
> > > > the order of dlopen'd units.
> > > > [...]
> > > > This patch is licensed under 2-clause BSD as per
> > > > winsup/CONTRIBUTORS, Copyright (c) 2017, MetaStack Solutions Ltd.
> 
> Do you really want to make it (c) MetaStack?

Oh, I was assuming that there would just be an implied mapping! Simple is best (MetaStack is just me, anyway!), so the previous patch and this fixup may be merged as my personal copyright, yes.

<snip>

> > > - Last but not least.  You add code to topsort so the loaded DLLs
> > >   are handled first.  The subsequent code is untouched.  However,
> > >   shouldn't the next loop then restrict calling populate_deps to the
> > >   linked DLLs only, at least for performance?
> >
> > Oops :$ That's an artefact of the "story" of the patch's development.
> > As it happens, the first dlopen'd DLL would have been initialised in
> > the second loop, not the first, but the presence of two loops like
> > that was indeed mostly inefficient. I've kept the original one as a
> > "fast path" for the case of no dlopen'd DLLs, though I don't know if
> > that's a worthwhile optimisation.
> 
> Well, interesting point.  Basically your new code is a drop-in
> replacement, except for the fact that it always calls an extra
> cmalloc/cfree.  However, this is only required if loaded_dlls > 0 so I
> think we may get away with removing the old loop with a simple tweak to
> your new one:
> 
>   dll** dlopen_deps = NULL;
>   if (loaded_dlls > 0)
>     dlopen_deps = (dll**) cmalloc (HEAP_2_DLL, maxdeps*sizeof (dll*));
>   while ((d = d->next))
>     {
>       [...]
>     }
>   if (dlopen_deps)
>     cfree (dlopen_deps);
> 
> Do you want to tweak your patch accordingly?

That's much neater - attached is a fixup (which obviously looks a lot clearer with git diff --ignore-all-space)

All best,


David

Attachment: 0001-Fixup-8607cf.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]