From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Memory leak due to bidi? Date: Wed, 03 Aug 2011 09:30:28 -0400 Message-ID: References: Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1312378241 18472 80.91.229.12 (3 Aug 2011 13:30:41 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Wed, 3 Aug 2011 13:30:41 +0000 (UTC) Cc: cloos@jhcloos.com, emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Aug 03 15:30:36 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1QobWt-0004Zv-3W for ged-emacs-devel@m.gmane.org; Wed, 03 Aug 2011 15:30:35 +0200 Original-Received: from localhost ([::1]:51003 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QobWr-00027M-OU for ged-emacs-devel@m.gmane.org; Wed, 03 Aug 2011 09:30:33 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:37383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QobWn-00025k-V9 for emacs-devel@gnu.org; Wed, 03 Aug 2011 09:30:31 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QobWm-00061V-51 for emacs-devel@gnu.org; Wed, 03 Aug 2011 09:30:29 -0400 Original-Received: from fencepost.gnu.org ([140.186.70.10]:40012) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QobWm-00061P-3E for emacs-devel@gnu.org; Wed, 03 Aug 2011 09:30:28 -0400 Original-Received: from eliz by fencepost.gnu.org with local (Exim 4.71) (envelope-from ) id 1QobWm-0000o5-0i; Wed, 03 Aug 2011 09:30:28 -0400 In-reply-to: (message from Stefan Monnier on Tue, 02 Aug 2011 21:10:14 -0400) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 140.186.70.10 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:142809 Archived-At: > From: Stefan Monnier > Cc: emacs-devel@gnu.org, Eli Zaretskii > Date: Tue, 02 Aug 2011 21:10:14 -0400 > > Let's not forget that it may have nothing to do with bidi. I just > happened to see bidi_shelve_cache as a "frequent" caller of mmap, but > according to Eli this is not necessarily abnormal. I would be the first to be happy if it turns out cache shelving and unshelving is not the culprit. However, as I still have my doubts, I instrumented the relevant code as shown in the patch below. With this patch, you can: . Put a breakpoint on some rarely-used interactive function (I use Fredraw_display), invoke it whenever you feel like it, and print the value of the variable bidi_cache_total_alloc -- it should be strictly zero, i.e. each allocation should be followed by a corresponding deallocation before Emacs gets back to its command loop. If you ever see a non-zero value, let alone a value that goes up, please proceed with the next item below. . Put a hardware watchpoint on bidi_cache_total_alloc, with the following commands: print bidi_cache_total_alloc bt 4 continue Then tell GDB to log all its output: set logging on and run Emacs until the value of bidi_cache_total_alloc again increases. Then send me the logfile (gdb.txt in the directory where you run GDB), or look into it to find some code that allocates a buffer, but never frees it. I am running with this patch for almost 3 hours, and the value of bidi_cache_total_alloc is strictly zero every time I look at it in the debugger. TIA Here's the patch: === modified file 'src/bidi.c' --- src/bidi.c 2011-08-02 19:16:32 +0000 +++ src/bidi.c 2011-08-03 10:24:32 +0000 @@ -620,12 +620,15 @@ bidi_pop_it (struct bidi_it *bidi_it) bidi_cache_last_idx = -1; } +ptrdiff_t bidi_cache_total_alloc; + /* Stash away a copy of the cache and its control variables. */ void * bidi_shelve_cache (void) { unsigned char *databuf; + /* Empty cache. */ if (bidi_cache_idx == 0) return NULL; @@ -634,6 +637,12 @@ bidi_shelve_cache (void) + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp) + sizeof (bidi_cache_start) + sizeof (bidi_cache_last_idx)); + bidi_cache_total_alloc += + sizeof (bidi_cache_idx) + bidi_cache_idx * sizeof (struct bidi_it) + + sizeof (bidi_cache_start_stack) + + sizeof (bidi_cache_sp) + sizeof (bidi_cache_start) + + sizeof (bidi_cache_last_idx); + memcpy (databuf, &bidi_cache_idx, sizeof (bidi_cache_idx)); memcpy (databuf + sizeof (bidi_cache_idx), bidi_cache, bidi_cache_idx * sizeof (struct bidi_it)); @@ -659,7 +668,7 @@ bidi_shelve_cache (void) /* Restore the cache state from a copy stashed away by bidi_shelve_cache. */ void -bidi_unshelve_cache (void *databuf) +bidi_unshelve_cache (void *databuf, int just_free) { unsigned char *p = databuf; @@ -672,30 +681,47 @@ bidi_unshelve_cache (void *databuf) } else { - memcpy (&bidi_cache_idx, p, sizeof (bidi_cache_idx)); - bidi_cache_ensure_space (bidi_cache_idx); - memcpy (bidi_cache, p + sizeof (bidi_cache_idx), - bidi_cache_idx * sizeof (struct bidi_it)); - memcpy (bidi_cache_start_stack, - p + sizeof (bidi_cache_idx) - + bidi_cache_idx * sizeof (struct bidi_it), - sizeof (bidi_cache_start_stack)); - memcpy (&bidi_cache_sp, - p + sizeof (bidi_cache_idx) - + bidi_cache_idx * sizeof (struct bidi_it) - + sizeof (bidi_cache_start_stack), - sizeof (bidi_cache_sp)); - memcpy (&bidi_cache_start, - p + sizeof (bidi_cache_idx) - + bidi_cache_idx * sizeof (struct bidi_it) - + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp), - sizeof (bidi_cache_start)); - memcpy (&bidi_cache_last_idx, - p + sizeof (bidi_cache_idx) - + bidi_cache_idx * sizeof (struct bidi_it) - + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp) - + sizeof (bidi_cache_start), - sizeof (bidi_cache_last_idx)); + if (just_free) + { + ptrdiff_t idx; + + memcpy (&idx, p, sizeof (bidi_cache_idx)); + bidi_cache_total_alloc -= + sizeof (bidi_cache_idx) + idx * sizeof (struct bidi_it) + + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp) + + sizeof (bidi_cache_start) + sizeof (bidi_cache_last_idx); + } + else + { + memcpy (&bidi_cache_idx, p, sizeof (bidi_cache_idx)); + bidi_cache_ensure_space (bidi_cache_idx); + memcpy (bidi_cache, p + sizeof (bidi_cache_idx), + bidi_cache_idx * sizeof (struct bidi_it)); + memcpy (bidi_cache_start_stack, + p + sizeof (bidi_cache_idx) + + bidi_cache_idx * sizeof (struct bidi_it), + sizeof (bidi_cache_start_stack)); + memcpy (&bidi_cache_sp, + p + sizeof (bidi_cache_idx) + + bidi_cache_idx * sizeof (struct bidi_it) + + sizeof (bidi_cache_start_stack), + sizeof (bidi_cache_sp)); + memcpy (&bidi_cache_start, + p + sizeof (bidi_cache_idx) + + bidi_cache_idx * sizeof (struct bidi_it) + + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp), + sizeof (bidi_cache_start)); + memcpy (&bidi_cache_last_idx, + p + sizeof (bidi_cache_idx) + + bidi_cache_idx * sizeof (struct bidi_it) + + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp) + + sizeof (bidi_cache_start), + sizeof (bidi_cache_last_idx)); + bidi_cache_total_alloc -= + sizeof (bidi_cache_idx) + bidi_cache_idx * sizeof (struct bidi_it) + + sizeof (bidi_cache_start_stack) + sizeof (bidi_cache_sp) + + sizeof (bidi_cache_start) + sizeof (bidi_cache_last_idx); + } xfree (p); } === modified file 'src/dispextern.h' --- src/dispextern.h 2011-08-02 19:16:32 +0000 +++ src/dispextern.h 2011-08-03 10:37:42 +0000 @@ -2978,7 +2978,7 @@ extern int bidi_mirror_char (int); extern void bidi_push_it (struct bidi_it *); extern void bidi_pop_it (struct bidi_it *); extern void *bidi_shelve_cache (void); -extern void bidi_unshelve_cache (void *); +extern void bidi_unshelve_cache (void *, int); /* Defined in xdisp.c */ === modified file 'src/dispnew.c' --- src/dispnew.c 2011-07-14 20:40:35 +0000 +++ src/dispnew.c 2011-08-03 10:32:00 +0000 @@ -5282,7 +5282,7 @@ buffer_posn_from_coords (struct window * argument is ZV to prevent move_it_in_display_line from matching based on buffer positions. */ move_it_in_display_line (&it, ZV, to_x, MOVE_TO_X); - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); Fset_buffer (old_current_buffer); === modified file 'src/indent.c' --- src/indent.c 2011-07-14 21:35:23 +0000 +++ src/indent.c 2011-08-03 10:32:13 +0000 @@ -2135,7 +2135,7 @@ whether or not it is currently displayed } SET_PT_BOTH (IT_CHARPOS (it), IT_BYTEPOS (it)); - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); } if (BUFFERP (old_buffer)) === modified file 'src/window.c' --- src/window.c 2011-07-14 17:28:42 +0000 +++ src/window.c 2011-08-03 10:35:05 +0000 @@ -1379,7 +1379,7 @@ if it isn't already recorded. */) if (it.current_y < it.last_visible_y) move_it_past_eol (&it); value = make_number (IT_CHARPOS (it)); - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); if (old_buffer) set_buffer_internal (old_buffer); @@ -4273,7 +4273,7 @@ window_scroll_pixel_based (Lisp_Object w } start = it.current.pos; - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); } else if (auto_window_vscroll_p) { @@ -4417,7 +4417,7 @@ window_scroll_pixel_based (Lisp_Object w } else { - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); if (noerror) return; else if (n < 0) /* could happen with empty buffers */ @@ -4434,7 +4434,7 @@ window_scroll_pixel_based (Lisp_Object w w->vscroll = 0; else { - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); if (noerror) return; else @@ -4583,7 +4583,7 @@ window_scroll_pixel_based (Lisp_Object w SET_PT_BOTH (charpos, bytepos); } } - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); } @@ -5010,7 +5010,7 @@ displayed_window_lines (struct window *w start_display (&it, w, start); move_it_vertically (&it, height); bottom_y = line_bottom_y (&it); - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); /* rms: On a non-window display, the value of it.vpos at the bottom of the screen @@ -5116,7 +5116,7 @@ and redisplay normally--don't erase and move_it_vertically_backward (&it, window_box_height (w) / 2); charpos = IT_CHARPOS (it); bytepos = IT_BYTEPOS (it); - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); } else if (iarg < 0) { @@ -5164,7 +5164,7 @@ and redisplay normally--don't erase and } if (h <= 0) { - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); return Qnil; } @@ -5187,7 +5187,7 @@ and redisplay normally--don't erase and charpos = IT_CHARPOS (it); bytepos = IT_BYTEPOS (it); - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); } else { === modified file 'src/xdisp.c' --- src/xdisp.c 2011-08-03 05:24:30 +0000 +++ src/xdisp.c 2011-08-03 10:36:28 +0000 @@ -604,7 +604,7 @@ int current_mode_line_height, current_he #define SAVE_IT(ITCOPY,ITORIG,CACHE) \ do { \ if (CACHE) \ - xfree (CACHE); \ + bidi_unshelve_cache (CACHE, 1); \ ITCOPY = ITORIG; \ CACHE = bidi_shelve_cache(); \ } while (0) @@ -613,7 +613,7 @@ int current_mode_line_height, current_he do { \ if (pITORIG != pITCOPY) \ *(pITORIG) = *(pITCOPY); \ - bidi_unshelve_cache (CACHE); \ + bidi_unshelve_cache (CACHE, 0); \ CACHE = NULL; \ } while (0) @@ -1341,9 +1341,9 @@ pos_visible_p (struct window *w, EMACS_I *vpos = it2.vpos; } else - xfree (it2data); + bidi_unshelve_cache (it2data, 1); } - bidi_unshelve_cache (itdata); + bidi_unshelve_cache (itdata, 0); if (old_buffer) set_buffer_internal_1 (old_buffer); @@ -2627,7 +2627,7 @@ init_iterator (struct it *it, struct win it->paragraph_embedding = R2L; else it->paragraph_embedding = NEUTRAL_DIR; - bidi_unshelve_cache (NULL); + bidi_unshelve_cache (NULL, 0); bidi_init_it (charpos, IT_BYTEPOS (*it), FRAME_WINDOW_P (it->f), &it->bidi_it); } @@ -5618,7 +5618,7 @@ back_to_previous_visible_line_start (str pos = --IT_CHARPOS (it2); --IT_BYTEPOS (it2); it2.sp = 0; - bidi_unshelve_cache (NULL); + bidi_unshelve_cache (NULL, 0); it2.string_from_display_prop_p = 0; it2.from_disp_prop_p = 0; if (handle_display_prop (&it2) == HANDLED_RETURN @@ -5828,7 +5828,7 @@ reseat_1 (struct it *it, struct text_pos { bidi_init_it (IT_CHARPOS (*it), IT_BYTEPOS (*it), FRAME_WINDOW_P (it->f), &it->bidi_it); - bidi_unshelve_cache (NULL); + bidi_unshelve_cache (NULL, 0); it->bidi_it.paragraph_dir = NEUTRAL_DIR; it->bidi_it.string.s = NULL; it->bidi_it.string.lstring = Qnil; @@ -8096,13 +8096,13 @@ move_it_in_display_line_to (struct it *i done: if (atpos_data) - xfree (atpos_data); + bidi_unshelve_cache (atpos_data, 1); if (atx_data) - xfree (atx_data); + bidi_unshelve_cache (atx_data, 1); if (wrap_data) - xfree (wrap_data); + bidi_unshelve_cache (wrap_data, 1); if (ppos_data) - xfree (ppos_data); + bidi_unshelve_cache (ppos_data, 1); /* Restore the iterator settings altered at the beginning of this function. */ @@ -8137,7 +8137,7 @@ move_it_in_display_line (struct it *it, (it, -1, prev_x, MOVE_TO_X); } else - xfree (save_data); + bidi_unshelve_cache (save_data, 1); } else move_it_in_display_line_to (it, to_charpos, to_x, op); @@ -8396,7 +8396,7 @@ move_it_to (struct it *it, EMACS_INT to_ } if (backup_data) - xfree (backup_data); + bidi_unshelve_cache (backup_data, 1); TRACE_MOVE ((stderr, "move_it_to: reached %d\n", reached)); } @@ -8475,7 +8475,7 @@ move_it_vertically_backward (struct it * RESTORE_IT (it, it, it2data); if (nlines > 0) move_it_by_lines (it, nlines); - xfree (it3data); + bidi_unshelve_cache (it3data, 1); } else { @@ -8671,7 +8671,7 @@ move_it_by_lines (struct it *it, int dvp if (IT_CHARPOS (*it) >= start_charpos) RESTORE_IT (it, &it2, it2data); else - xfree (it2data); + bidi_unshelve_cache (it2data, 1); } else RESTORE_IT (it, it, it2data);