Improve code in tidbitmap.c

Started by Etsuro Fujitaabout 12 years ago4 messages

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

about 12 years ago

1 attachment(s)

Hi,

ISTM the code in tidbitmap.c should be improved. Patch attached. I think
this patch increases the efficiency a bit.

Thanks,

Best regards,

Etsuro Fujita

Attachments:

tidbitmap-20131108.patch.txttext/plain; name=tidbitmap-20131108.patch.txtDownload

diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index 3ef0112..43628ac 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -361,7 +361,7 @@ tbm_union_page(TIDBitmap *a, const PagetableEntry *bpage)
        if (bpage->ischunk)
        {
                /* Scan b's chunk, mark each indicated page lossy in a */
-               for (wordnum = 0; wordnum < WORDS_PER_PAGE; wordnum++)
+               for (wordnum = 0; wordnum < WORDS_PER_CHUNK; wordnum++)
                {
                        bitmapword      w = bpage->words[wordnum];

@@ -473,7 +473,7 @@ tbm_intersect_page(TIDBitmap *a, PagetableEntry *apage, const TIDBitmap *b)
                /* Scan each bit in chunk, try to clear */
                bool            candelete = true;

-               for (wordnum = 0; wordnum < WORDS_PER_PAGE; wordnum++)
+               for (wordnum = 0; wordnum < WORDS_PER_CHUNK; wordnum++)
                {
                        bitmapword      w = apage->words[wordnum];

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

about 12 years ago

In reply to: Etsuro Fujita (#1)

Re: Improve code in tidbitmap.c

I'd like to do the complementary explanation of this.

In tbm_union_page() and tbm_intersect_page() in tidbitmap.c, WORDS_PER_PAGE
is used in the scan of a lossy chunk, instead of WORDS_PER_CHUNK, where
these macros are defined as:

/* number of active words for an exact page: */
#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD +
1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)

Though the use of WORDS_PER_PAGE in the scan of a lossy chunk is logically
correct since these macros implicitly satisfy that WORDS_PER_CHUNK <
WORDS_PER_PAGE, I think WORDS_PER_CHUNK should be used in the scan of a
lossy chunk for code readability and maintenance. So, I submitted a patch
working in such a way in an earlier email.

I think that, as a secondary effect of the patch, the scan would be
performed a bit efficiently.

I'll add the patch to the 2013-11 CF. Any comments are welcome.

Thanks,

Best regards,
Etsuro Fujita

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Rowley

dgrowleyml@gmail.com

about 12 years ago

In reply to: Etsuro Fujita (#2)

Re: Improve code in tidbitmap.c

On Fri, Nov 15, 2013 at 12:50 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp

wrote:

I think that, as a secondary effect of the patch, the scan would be
performed a bit efficiently.

Hi Etsuro,

Would you be able to supply some benchmarks of a work load which benefits
from this change?
Something which compares patched and unpatched versions of head would be
really good.

Regards

David Rowley

Show quoted text

I'll add the patch to the 2013-11 CF. Any comments are welcome.

Thanks,

Best regards,
Etsuro Fujita

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Import Notes

Reply to msg id not found: 5284b959.c44b0f0a.555e.ffffc942SMTPIN_ADDED_BROKEN@mx.google.comReference msg id not found: 5284b959.c44b0f0a.555e.ffffc942SMTPIN_ADDED_BROKEN@mx.google.com | Resolved by subject fallback

Tom Lane

tgl@sss.pgh.pa.us

about 12 years ago

In reply to: Etsuro Fujita (#2)

Re: Improve code in tidbitmap.c

"Etsuro Fujita" <fujita.etsuro@lab.ntt.co.jp> writes:

I'd like to do the complementary explanation of this.
In tbm_union_page() and tbm_intersect_page() in tidbitmap.c, WORDS_PER_PAGE
is used in the scan of a lossy chunk, instead of WORDS_PER_CHUNK, where
these macros are defined as:

/* number of active words for an exact page: */
#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD +
1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)

Though the use of WORDS_PER_PAGE in the scan of a lossy chunk is logically
correct since these macros implicitly satisfy that WORDS_PER_CHUNK <
WORDS_PER_PAGE, I think WORDS_PER_CHUNK should be used in the scan of a
lossy chunk for code readability and maintenance. So, I submitted a patch
working in such a way in an earlier email.

This is a bug fix, not a performance improvement (any improvement would
be welcome, but that's not the point). There's absolutely nothing
guaranteeing that WORDS_PER_CHUNK is less than WORDS_PER_PAGE, and if
it were the other way around, the code would be outright broken. It's
pure luck that it was merely inefficient.

Committed, thanks for finding it!

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers