Re: Curious... on readline history patch -- round up memory allocs?

[Chet Ramey]

On 11/15/16 7:33 PM, L. A. Walsh wrote:

> ---
> That's fine -- I was just reading the comment about how 4.4 changed
> history allocs to optimize memory reallocs and movement.  So if things
> were fine with malloc before, I'm not sure why they were optimized
> within bash in 4.4...?

In the general case, realloc has to (internally) allocate a new block,
copy the contents from the old block to the new one, and free the old
block.  Reducing the number of reallocs reduces the number of potential
memory copies, which improves both memory locality and speed.

This doesn't have anything to do with aligning requests for memory on
page boundaries, which as Eduardo correctly notes, is the responsibility
of the malloc library layer.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: Curious... on readline history patch -- round up memory allocs?

[Eduardo Bustamante]

The change was in reference to this bug report:
http://lists.gnu.org/archive/html/bug-bash/2016-09/msg00107.html

The problem was that bash tried to allocate memory from the start
based on the value of HISTSIZE, but this proved problematic for users
which used a large HISTSIZE to have unlimited history.

The commit is here
http://git.savannah.gnu.org/cgit/bash.git/commit/?h=devel=61c476d20d32dfd389c79fd4f2161a780685e42e

And the entry in the changelog:

+lib/readine/history.c
+ - add_history: if allocating the history list for the first time,
+ make sure the max history list size isn't so large that it will
+ cause allocation errors. Cap it at MAX_HISTORY_INITIAL_SIZE
+ (8192). Reported by Sean Zha 

Re: Curious... on readline history patch -- round up memory allocs?

[L. A. Walsh]

Eduardo Bustamante wrote:

I think this is unnecessary, malloc (either the bash malloc in
lib/malloc/malloc.c or the libc provided malloc) should already take
care of requesting memory in page sized chunks.

At least that's what I see here (morecore function):
http://git.savannah.gnu.org/cgit/bash.git/tree/lib/malloc/malloc.c?h=devel=216e2e9b8ba21fff677cf7794ef3d9af8c91d46d#n622

Unless I'm missing something, there's nothing to gain from optimizing
on top of xmalloc/malloc.
  

---
That's fine -- I was just reading the comment about how 4.4 changed
history allocs to optimize memory reallocs and movement.  So if things
were fine with malloc before, I'm not sure why they were optimized
within bash in 4.4...?

Re: Curious... on readline history patch -- round up memory allocs?

[Eduardo Bustamante]

I think this is unnecessary, malloc (either the bash malloc in
lib/malloc/malloc.c or the libc provided malloc) should already take
care of requesting memory in page sized chunks.

At least that's what I see here (morecore function):
http://git.savannah.gnu.org/cgit/bash.git/tree/lib/malloc/malloc.c?h=devel=216e2e9b8ba21fff677cf7794ef3d9af8c91d46d#n622

Unless I'm missing something, there's nothing to gain from optimizing
on top of xmalloc/malloc.

Curious... on readline history patch -- round up memory allocs?

[L. A. Walsh]

Saw the bit about bash-4.4 changing things to reduce reallocs/copies,
and wondered if you'd thought about rounding up the allocations
to the nearest page size (at least on linux):

Something along the lines of:
Ishtar:tools/bash/readline-7.0> diff -u history.c.orig history.c
--- history.c 2015-12-28 10:50:31.0 -0800
+++ history.c 2016-11-15 12:36:51.452105223 -0800
@@ -57,6 +57,12 @@
/* How big to make the_history when we first allocate it. */
#define DEFAULT_HISTORY_INITIAL_SIZE 502

+#define MAX_HISTORY_INITIAL_SIZE 8192
+
+#define DEFAULT_PAGE_SIZE 8192
+
+#define ROUND_UP_TO_PAGE_SIZE(x) ( x + DEFAULT_PAGE_SIZE-1) & 
~(DEFAULT_PAGE_SIZE-1)

+
/* The number of slots to increase the_history by. */
#define DEFAULT_HISTORY_GROW_SIZE 50

@@ -310,16 +316,19 @@
 history_size = history_max_entries + 2;
   else
 history_size = DEFAULT_HISTORY_INITIAL_SIZE;
-   the_history = (HIST_ENTRY **)xmalloc (history_size * sizeof 
(HIST_ENTRY *));
+   the_history = (HIST_ENTRY **)xmalloc 
(ROUND_UP_TO_PAGE_SIZE(history_size * sizeof (HIST_ENTRY *)));

   history_length = 1;
 }
  else
 {
   if (history_length == (history_size - 1))
 {
-   history_size += DEFAULT_HISTORY_GROW_SIZE;
+   history_size = (history_max_entries > MAX_HISTORY_INITIAL_SIZE)
+ ? MAX_HISTORY_INITIAL_SIZE
+ : history_max_entries + 2;
   the_history = (HIST_ENTRY **)
-   xrealloc (the_history, history_size * sizeof (HIST_ENTRY *));
+   xrealloc (the_history, ROUND_UP_TO_PAGE_SIZE(history_size * sizeof 
(HIST_ENTRY *)));

+
 }
   history_length++;
 }


Note, since I'm having probs building bash right now, the above
isn't execution tested, but it does compile!  ;-)