rc: reorder_libs: [2/2] Pick archive versions more efficiently

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

rc: reorder_libs: [2/2] Pick archive versions more efficiently

Klemens Nanni
Why looping over all existing archives, picking the latest version of
the current archive, skipping it in case it's already in our list of
selected latest versions or adding it otherwise?

The current code runs ls|sort|tail about n * (v - 1) times for n
different libraries and v versions respectively since the globbed list
is almost always sorted already, effectively adding the latest versions
after skipping all others.

This diff makes it much clearer and simpler by sorting and picking
only as many versions as there are libraries to reorder (two). Globbing
is done within the loop so future libraries with different naming
schemes comes at no cost.

Applies cleanly to both the current revision as well as my previous diff
but the previous one will fail on top of this one.

Feedback? Comments?

Index: rc
===================================================================
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 -0000 1.507
+++ rc 16 Jul 2017 01:15:43 -0000
@@ -171,13 +171,10 @@ reorder_libs() {
  echo -n 'reordering libraries:'
 
  # Only choose the latest version of the libraries.
- for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
- _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
- for _l in $_libas; do
- [[ $_l == $_liba ]] && continue 2
- done
- _libas="$_libas $_liba"
+ for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
+ _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
  done
+ _libas=${_libas# }
 
  # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
  if [[ $_mp == *' type ffs '*'read-only'* ]]; then

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Theo Buehler
On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:

> Why looping over all existing archives, picking the latest version of
> the current archive, skipping it in case it's already in our list of
> selected latest versions or adding it otherwise?
>
> The current code runs ls|sort|tail about n * (v - 1) times for n
> different libraries and v versions respectively since the globbed list
> is almost always sorted already, effectively adding the latest versions
> after skipping all others.
>
> This diff makes it much clearer and simpler by sorting and picking
> only as many versions as there are libraries to reorder (two). Globbing
> is done within the loop so future libraries with different naming
> schemes comes at no cost.
>
> Applies cleanly to both the current revision as well as my previous diff
> but the previous one will fail on top of this one.
>
> Feedback? Comments?

I like this. It's much clearer.

I think it's ok to commit this, but I wonder if we should test it in
snapshots first.

>
> Index: rc
> ===================================================================
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc 4 Jul 2017 19:02:11 -0000 1.507
> +++ rc 16 Jul 2017 01:15:43 -0000
> @@ -171,13 +171,10 @@ reorder_libs() {
>   echo -n 'reordering libraries:'
>  
>   # Only choose the latest version of the libraries.
> - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> - for _l in $_libas; do
> - [[ $_l == $_liba ]] && continue 2
> - done
> - _libas="$_libas $_liba"
> + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
>   done
> + _libas=${_libas# }
>  
>   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>   if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Robert Peichaer
In reply to this post by Klemens Nanni
On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> Why looping over all existing archives, picking the latest version of
> the current archive, skipping it in case it's already in our list of
> selected latest versions or adding it otherwise?
>
> The current code runs ls|sort|tail about n * (v - 1) times for n
> different libraries and v versions respectively since the globbed list
> is almost always sorted already, effectively adding the latest versions
> after skipping all others.

Yeah well, until the globbing isn't sorting as expected.

$ ls -1 libc.so.*.a
libc.so.89.10.a
libc.so.89.6.a
libc.so.89.7.a
libc.so.89.8.a
libc.so.89.9.a

But that's not the point anyway in your otherwise perfectly valid change.
 

> This diff makes it much clearer and simpler by sorting and picking
> only as many versions as there are libraries to reorder (two). Globbing
> is done within the loop so future libraries with different naming
> schemes comes at no cost.
>
> Applies cleanly to both the current revision as well as my previous diff
> but the previous one will fail on top of this one.
>
> Feedback? Comments?
>
> Index: rc
> ===================================================================
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc 4 Jul 2017 19:02:11 -0000 1.507
> +++ rc 16 Jul 2017 01:15:43 -0000
> @@ -171,13 +171,10 @@ reorder_libs() {
>   echo -n 'reordering libraries:'
>  
>   # Only choose the latest version of the libraries.
> - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> - for _l in $_libas; do
> - [[ $_l == $_liba ]] && continue 2
> - done
> - _libas="$_libas $_liba"
> + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
>   done

That's definitely a much better approach.

But I'd like to stay strict matching the filenames.

+ for _liba in /usr/lib/lib{c,crypto}; do
+ _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
+ done

> + _libas=${_libas# }

This would be another way of suppressing the blank right away.
But it's not really necessary anyway, because the leading blank is
removed in the list of the other for-loop.

        _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
 
>   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>   if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>

--
-=[rpe]=-

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Theo Buehler
> But I'd like to stay strict matching the filenames.
>
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done

Yes, this is indeed better.

So Klemens, can you write a patch that uses this and removes the then
unused _l. We can think about the hoisting in a second step.

> > + _libas=${_libas# }
>
> This would be another way of suppressing the blank right away.
> But it's not really necessary anyway, because the leading blank is
> removed in the list of the other for-loop.
>
> _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"

While this is is clever, it does a bit too much in one line for my taste.

I'm fine with Klemens's version of zapping the blank after the for loop,
but we can also omit this, I don't really care.

>  
> >   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> >   if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> >
>
> --
> -=[rpe]=-
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Klemens Nanni
In reply to this post by Robert Peichaer
On Sun, Jul 16, 2017 at 10:26:25AM +0000, Robert Peichaer wrote:

> On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> > Why looping over all existing archives, picking the latest version of
> > the current archive, skipping it in case it's already in our list of
> > selected latest versions or adding it otherwise?
> >
> > The current code runs ls|sort|tail about n * (v - 1) times for n
> > different libraries and v versions respectively since the globbed list
> > is almost always sorted already, effectively adding the latest versions
> > after skipping all others.
>
> Yeah well, until the globbing isn't sorting as expected.
>
> $ ls -1 libc.so.*.a
> libc.so.89.10.a
> libc.so.89.6.a
> libc.so.89.7.a
> libc.so.89.8.a
> libc.so.89.9.a
Exactly, that's why sort(1) is used.

>
> But that's not the point anyway in your otherwise perfectly valid change.
>  
> > This diff makes it much clearer and simpler by sorting and picking
> > only as many versions as there are libraries to reorder (two). Globbing
> > is done within the loop so future libraries with different naming
> > schemes comes at no cost.
> >
> > Applies cleanly to both the current revision as well as my previous diff
> > but the previous one will fail on top of this one.
> >
> > Feedback? Comments?
> >
> > Index: rc
> > ===================================================================
> > RCS file: /cvs/src/etc/rc,v
> > retrieving revision 1.507
> > diff -u -p -r1.507 rc
> > --- rc 4 Jul 2017 19:02:11 -0000 1.507
> > +++ rc 16 Jul 2017 01:15:43 -0000
> > @@ -171,13 +171,10 @@ reorder_libs() {
> >   echo -n 'reordering libraries:'
> >  
> >   # Only choose the latest version of the libraries.
> > - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> > - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> > - for _l in $_libas; do
> > - [[ $_l == $_liba ]] && continue 2
> > - done
> > - _libas="$_libas $_liba"
> > + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> > + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
> >   done
>
> That's definitely a much better approach.
>
> But I'd like to stay strict matching the filenames.
>
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done
>
> > + _libas=${_libas# }
>
> This would be another way of suppressing the blank right away.
> But it's not really necessary anyway, because the leading blank is
> removed in the list of the other for-loop.
>
> _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
I explicitly avoided this since stripping the leading space in the end
seemed clearer about what's going on. Also, this check is done for each
library whereas only the first one is true.

I also thought about leaving it in but stripping it makes clear I know
what I'm doing and future changes where $_libas might get quoted will
be working as expected.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Klemens Nanni
In reply to this post by Robert Peichaer
On Sun, Jul 16, 2017 at 10:26:25AM +0000, Robert Peichaer wrote:
> But I'd like to stay strict matching the filenames.
>
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done
>
> > + _libas=${_libas# }
Agreed, I had a similiar approach first but then tried to reduce the
differences to the essentials.

Here's an updated diff taking this into while also dropping $_l together
with this hunk instead of the other one.

Feedback?

Index: rc
===================================================================
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 -0000 1.507
+++ rc 16 Jul 2017 11:54:36 -0000
@@ -158,7 +158,7 @@ make_keys() {
 
 # Re-link libraries, placing the objects in a random order.
 reorder_libs() {
- local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
+ local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
 
  [[ $library_aslr == NO ]] && return
 
@@ -171,13 +171,10 @@ reorder_libs() {
  echo -n 'reordering libraries:'
 
  # Only choose the latest version of the libraries.
- for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
- _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
- for _l in $_libas; do
- [[ $_l == $_liba ]] && continue 2
- done
- _libas="$_libas $_liba"
+ for _liba in /usr/lib/lib{c,crypto}; do
+ _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
  done
+ _libas=${_libas# }
 
  # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
  if [[ $_mp == *' type ffs '*'read-only'* ]]; then

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Robert Peichaer
In reply to this post by Klemens Nanni
On Sun, Jul 16, 2017 at 01:23:00PM +0200, Klemens Nanni wrote:

> On Sun, Jul 16, 2017 at 10:26:25AM +0000, Robert Peichaer wrote:
> > On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> > > Why looping over all existing archives, picking the latest version of
> > > the current archive, skipping it in case it's already in our list of
> > > selected latest versions or adding it otherwise?
> > >
> > > The current code runs ls|sort|tail about n * (v - 1) times for n
> > > different libraries and v versions respectively since the globbed list
> > > is almost always sorted already, effectively adding the latest versions
> > > after skipping all others.
> >
> > Yeah well, until the globbing isn't sorting as expected.
> >
> > $ ls -1 libc.so.*.a
> > libc.so.89.10.a
> > libc.so.89.6.a
> > libc.so.89.7.a
> > libc.so.89.8.a
> > libc.so.89.9.a
> Exactly, that's why sort(1) is used.
> >
> > But that's not the point anyway in your otherwise perfectly valid change.
> >  
> > > This diff makes it much clearer and simpler by sorting and picking
> > > only as many versions as there are libraries to reorder (two). Globbing
> > > is done within the loop so future libraries with different naming
> > > schemes comes at no cost.
> > >
> > > Applies cleanly to both the current revision as well as my previous diff
> > > but the previous one will fail on top of this one.
> > >
> > > Feedback? Comments?
> > >
> > > Index: rc
> > > ===================================================================
> > > RCS file: /cvs/src/etc/rc,v
> > > retrieving revision 1.507
> > > diff -u -p -r1.507 rc
> > > --- rc 4 Jul 2017 19:02:11 -0000 1.507
> > > +++ rc 16 Jul 2017 01:15:43 -0000
> > > @@ -171,13 +171,10 @@ reorder_libs() {
> > >   echo -n 'reordering libraries:'
> > >  
> > >   # Only choose the latest version of the libraries.
> > > - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> > > - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> > > - for _l in $_libas; do
> > > - [[ $_l == $_liba ]] && continue 2
> > > - done
> > > - _libas="$_libas $_liba"
> > > + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> > > + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
> > >   done
> >
> > That's definitely a much better approach.
> >
> > But I'd like to stay strict matching the filenames.
> >
> > + for _liba in /usr/lib/lib{c,crypto}; do
> > + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> > + done
> >
> > > + _libas=${_libas# }
> >
> > This would be another way of suppressing the blank right away.
> > But it's not really necessary anyway, because the leading blank is
> > removed in the list of the other for-loop.
> >
> > _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> I explicitly avoided this since stripping the leading space in the end
> seemed clearer about what's going on. Also, this check is done for each
> library whereas only the first one is true.
>
> I also thought about leaving it in but stripping it makes clear I know
> what I'm doing and future changes where $_libas might get quoted will
> be working as expected.

To the contrary what my previous answer might indicate, I don't care
that much either. If you want to explicitely remove the blank, go for
it.

--
-=[rpe]=-

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Robert Peichaer
In reply to this post by Klemens Nanni
On Sun, Jul 16, 2017 at 01:55:02PM +0200, Klemens Nanni wrote:

> On Sun, Jul 16, 2017 at 10:26:25AM +0000, Robert Peichaer wrote:
> > But I'd like to stay strict matching the filenames.
> >
> > + for _liba in /usr/lib/lib{c,crypto}; do
> > + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> > + done
> >
> > > + _libas=${_libas# }
> Agreed, I had a similiar approach first but then tried to reduce the
> differences to the essentials.
>
> Here's an updated diff taking this into while also dropping $_l together
> with this hunk instead of the other one.
>
> Feedback?
>
> Index: rc
> ===================================================================
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc 4 Jul 2017 19:02:11 -0000 1.507
> +++ rc 16 Jul 2017 11:54:36 -0000
> @@ -158,7 +158,7 @@ make_keys() {
>  
>  # Re-link libraries, placing the objects in a random order.
>  reorder_libs() {
> - local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
> + local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
>  
>   [[ $library_aslr == NO ]] && return
>  
> @@ -171,13 +171,10 @@ reorder_libs() {
>   echo -n 'reordering libraries:'
>  
>   # Only choose the latest version of the libraries.
> - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> - for _l in $_libas; do
> - [[ $_l == $_liba ]] && continue 2
> - done
> - _libas="$_libas $_liba"
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
>   done
> + _libas=${_libas# }
>  
>   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>   if [[ $_mp == *' type ffs '*'read-only'* ]]; then

Provided you tested this throughly, OK.

--
-=[rpe]=-

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Klemens Nanni
In reply to this post by Robert Peichaer
On Sun, Jul 16, 2017 at 11:55:09AM +0000, Robert Peichaer wrote:
> To the contrary what my previous answer might indicate, I don't care
> that much either. If you want to explicitely remove the blank, go for
> it.
Most problems I ever encountered while writing shell code were related
to (nested) quoting and unexpected characters (mostly whitespaces).

Leaving the resulting $_libas exactly as one would expect it to be
doesn't hurt plus the code's intent is perfectly clear while reading it.

See the updated diff.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Vadim Zhukov
In reply to this post by Klemens Nanni
2017-07-16 14:55 GMT+03:00 Klemens Nanni <[hidden email]>:

> On Sun, Jul 16, 2017 at 10:26:25AM +0000, Robert Peichaer wrote:
>> But I'd like to stay strict matching the filenames.
>>
>> +     for _liba in /usr/lib/lib{c,crypto}; do
>> +             _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
>> +     done
>>
>> > +   _libas=${_libas# }
> Agreed, I had a similiar approach first but then tried to reduce the
> differences to the essentials.
>
> Here's an updated diff taking this into while also dropping $_l together
> with this hunk instead of the other one.
>
> Feedback?
>
> Index: rc
> ===================================================================
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc  4 Jul 2017 19:02:11 -0000       1.507
> +++ rc  16 Jul 2017 11:54:36 -0000
> @@ -158,7 +158,7 @@ make_keys() {
>
>  # Re-link libraries, placing the objects in a random order.
>  reorder_libs() {
> -       local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
> +       local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
>
>         [[ $library_aslr == NO ]] && return
>
> @@ -171,13 +171,10 @@ reorder_libs() {
>         echo -n 'reordering libraries:'
>
>         # Only choose the latest version of the libraries.
> -       for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> -               _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> -               for _l in $_libas; do
> -                       [[ $_l == $_liba ]] && continue 2
> -               done
> -               _libas="$_libas $_liba"
> +       for _liba in /usr/lib/lib{c,crypto}; do
> +               _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
>         done
> +       _libas=${_libas# }
>
>         # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>         if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>

As a matter of microoptimization we could use "sort -rV | head -1"
instead of "sort -V | tail -1". I'm okay with current version of this
diff already, though.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Klemens Nanni
On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:

> > +       for _liba in /usr/lib/lib{c,crypto}; do
> > +               _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> >         done
> > +       _libas=${_libas# }
> >
> >         # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> >         if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> >
>
> As a matter of microoptimization we could use "sort -rV | head -1"
> instead of "sort -V | tail -1". I'm okay with current version of this
> diff already, though.
Sorting requires to read the entire input, otherwise you'd only sort a
subset of the input. I don't see anything being optimized here.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Vadim Zhukov
2017-07-17 14:03 GMT+03:00 Klemens Nanni <[hidden email]>:

> On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:
>> > +       for _liba in /usr/lib/lib{c,crypto}; do
>> > +               _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
>> >         done
>> > +       _libas=${_libas# }
>> >
>> >         # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>> >         if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>> >
>>
>> As a matter of microoptimization we could use "sort -rV | head -1"
>> instead of "sort -V | tail -1". I'm okay with current version of this
>> diff already, though.
> Sorting requires to read the entire input, otherwise you'd only sort a
> subset of the input. I don't see anything being optimized here.

head -1 returns earlier than tail -1. ;)

--
  WBR,
  Vadim Zhukov

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Klemens Nanni
On Mon, Jul 17, 2017 at 02:14:26PM +0300, Vadim Zhukov wrote:

> 2017-07-17 14:03 GMT+03:00 Klemens Nanni <[hidden email]>:
> > On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:
> >> > +       for _liba in /usr/lib/lib{c,crypto}; do
> >> > +               _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> >> >         done
> >> > +       _libas=${_libas# }
> >> >
> >> >         # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> >> >         if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> >> >
> >>
> >> As a matter of microoptimization we could use "sort -rV | head -1"
> >> instead of "sort -V | tail -1". I'm okay with current version of this
> >> diff already, though.
> > Sorting requires to read the entire input, otherwise you'd only sort a
> > subset of the input. I don't see anything being optimized here.
>
> head -1 returns earlier than tail -1. ;)
Generally speaking, yes. But here, even for thousands of lines to be
sorted this won't make a difference. For me it's just changing logic
without benefit.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

Theo Buehler
In reply to this post by Vadim Zhukov
On Mon, Jul 17, 2017 at 02:14:26PM +0300, Vadim Zhukov wrote:

> 2017-07-17 14:03 GMT+03:00 Klemens Nanni <[hidden email]>:
> > On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:
> >> > +       for _liba in /usr/lib/lib{c,crypto}; do
> >> > +               _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> >> >         done
> >> > +       _libas=${_libas# }
> >> >
> >> >         # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> >> >         if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> >> >
> >>
> >> As a matter of microoptimization we could use "sort -rV | head -1"
> >> instead of "sort -V | tail -1". I'm okay with current version of this
> >> diff already, though.
> > Sorting requires to read the entire input, otherwise you'd only sort a
> > subset of the input. I don't see anything being optimized here.
>
> head -1 returns earlier than tail -1. ;)

I had thought about suggesting this as well, but then I felt it's not
worth it: we're talking about a couple of dozen lines max anyway.

I committed the diff as it is, thanks!

Loading...