[PATCH] Fix memory-report--object-size for hashtable and vectors

[PATCH] Fix memory-report--object-size for hashtable and vectors

[Yikai Zhao]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Hi,

I'd like to send a simple patch to fix the `memory-report--object-size' function in memory-report.el. Currently it did not give correct object size for hashtable and vector objects, this patch fixes that.

I noticed this bug because when I did `memory-report', the result items in "Largest Variables" list were unreasonably small and it did not match the "Overall Object Memory Usage". With this patch, this should be solved.

To verify this, execute the following code:

(memory-report--object-size
 (make-hash-table :test #'eq)
 ["long string that should be at least 40 bytes"])

The expected output should be a number greater than 40.

This is my first attempt to submit a patch to emacs, please let me know if there's anything missing. Thanks!


Yikai

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-memory-report-object-size-for-hashtable-and-vect.patch --]
[-- Type: text/x-patch; name="0001-Fix-memory-report-object-size-for-hashtable-and-vect.patch", Size: 1462 bytes --]

From cbec8009be8c7c49607ec3fe06fbba2178ed2f06 Mon Sep 17 00:00:00 2001
From: BlahGeek <i@blahgeek.com>
Date: Sat, 14 Aug 2021 18:01:17 +0800
Subject: [PATCH] Fix memory-report--object-size for hashtable and vectors

Previously the elements get inserted to the 'counted' table *before*
they get accounted. That operation is unnecessary because it will be
done at the beginning of `memory-report--object-size`
---
 lisp/emacs-lisp/memory-report.el | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/memory-report.el b/lisp/emacs-lisp/memory-report.el
index 1125dde405..1815deec40 100644
--- a/lisp/emacs-lisp/memory-report.el
+++ b/lisp/emacs-lisp/memory-report.el
@@ -230,7 +230,7 @@ memory-report--object-size-1
   (let ((total (+ (memory-report--size 'vector)
                   (* (memory-report--size 'object) (length value)))))
     (cl-loop for elem across value
-             do (setf (gethash elem counted) t)
+             do
              (cl-incf total (memory-report--object-size counted elem)))
     total))
 
@@ -239,8 +239,6 @@ memory-report--object-size-1
                   (* (memory-report--size 'object) (hash-table-size value)))))
     (maphash
      (lambda (key elem)
-       (setf (gethash key counted) t)
-       (setf (gethash elem counted) t)
        (cl-incf total (memory-report--object-size counted key))
        (cl-incf total (memory-report--object-size counted elem)))
      value)
-- 
2.32.0

Re: [PATCH] Fix memory-report--object-size for hashtable and vectors

[Lars Ingebrigtsen]

"Yikai Zhao" <i@blahgeek.com> writes:

> This is my first attempt to submit a patch to emacs, please let me
> know if there's anything missing. Thanks!

Thanks; looks good.  I added some tests to
test/lisp/emacs-lisp/memory-report-tests.el and pushed to Emacs 28.

This change was small enough to apply without assigning copyright to the
FSF, but for future patches you want to submit, it might make sense to
get the paperwork started now, so that subsequent patches can be applied
speedily. Would you be willing to sign such paperwork?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix memory-report--object-size for hashtable and vectors

[Yikai Zhao]

On Sat, Aug 14, 2021, at 19:46, Lars Ingebrigtsen wrote:
> Thanks; looks good.  I added some tests to
> test/lisp/emacs-lisp/memory-report-tests.el and pushed to Emacs 28.

Thanks!

> This change was small enough to apply without assigning copyright to the
> FSF, but for future patches you want to submit, it might make sense to
> get the paperwork started now, so that subsequent patches can be applied
> speedily. Would you be willing to sign such paperwork?

Yes, I'd like to sign it. Please send me the paperwork.

Yikai

Re: [PATCH] Fix memory-report--object-size for hashtable and vectors

[Lars Ingebrigtsen]

"Yikai Zhao" <i@blahgeek.com> writes:

>> This change was small enough to apply without assigning copyright to the
>> FSF, but for future patches you want to submit, it might make sense to
>> get the paperwork started now, so that subsequent patches can be applied
>> speedily. Would you be willing to sign such paperwork?
>
> Yes, I'd like to sign it. Please send me the paperwork.

Great!  Here's the form to get started:


Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]