public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 7/8] cirrus/ccache: disable compression and show stats
5+ messages / 4 participants
[nested] [flat]

* [PATCH 7/8] cirrus/ccache: disable compression and show stats
@ 2022-04-03 05:10  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Justin Pryzby @ 2022-04-03 05:10 UTC (permalink / raw)

Since v4.0, ccache enables zstd compression by default, saving roughly
2x-3x.  But, cirrus caches are compressed as tar.gz, so we could disable
ccache compression, allowing cirrus to gzip the uncompressed data
(better than ccache's default of zstd-1).

With default compression enabled (https://cirrus-ci.com/task/6692342840164352):
linux/debian/bullseye has 4.2; 99MB cirrus cache; cache_size_kibibyte	109616
macos has 4.5.1: 47MB cirrus cache; cache_size_kibibyte	52500
freebsd has 3.7.12: 42MB cirrus cache; cache_size_kibibyte	134064
XXX windows has 4.7.2; 180MB cirrus cache; cache_size_kibibyte	51179
todo: compiler warnings

With compression disabled (https://cirrus-ci.com/task/4614182514458624):
linux: 91MB cirrus cache; cache_size_kibibyte	316136
macos: 41MB cirrus cache; cache_size_kibibyte	118068
windows: 42MB cirrus cache; cache_size_kibibyte	134064
freebsd is the same

The stats should be shown and/or logged.
ccache --show-stats shows the *cumulative* stats (including prior
compilations)
ccache --zero-stats clears out not only the global stats, but the
per-file cache stats (from which the global stats are derived) - which
obviously makes the cache work poorly.

Note that ccache 4.4 supports CCACHE_STATSLOG, which seems ideal.
The log should be written *outside* the ccache folder - it shouldn't be
preserved across cirrusci task invocations.

freebsd, linux, macos
ci-os-only: linux
---
 .cirrus.yml | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 221431e70c4..d1c3119e4ff 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -16,7 +16,9 @@ env:
   # Useful to be able to analyse what in a script takes long
   CIRRUS_LOG_TIMESTAMP: true
 
-  CCACHE_MAXSIZE: "250M"
+  CCACHE_MAXSIZE: "750M"
+  CCACHE_NOCOMPRESS: 1
+  #CCACHE_STATSLOG: ccache-stats.log
 
   # target to test, for all but windows
   CHECK: check-world PROVE_FLAGS=$PROVE_FLAGS
@@ -236,7 +238,7 @@ task:
     - name: Linux - Debian Bullseye - Meson
 
       env:
-        CCACHE_MAXSIZE: "400M" # tests two different builds
+        CCACHE_MAXSIZE: "1200M" # tests two different builds
 
       configure_script: |
         su postgres <<-EOF
@@ -307,6 +309,7 @@ task:
 
     CIRRUS_WORKING_DIR: ${HOME}/pgsql/
     CCACHE_DIR: ${HOME}/ccache
+    CCACHE_STATSLOG: ${CCACHE_DIR}.stats.log
     HOMEBREW_CACHE: ${HOME}/homebrew-cache
     PERL5LIB: ${HOME}/perl5/lib/perl5
 
@@ -388,7 +391,7 @@ task:
       -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
       build
 
-  build_script: ninja -C build -j${BUILD_JOBS}
+  build_script: ninja -C build -j${BUILD_JOBS} && ccache --show-log-stats
   upload_caches: ccache
 
   test_world_script: |
@@ -496,14 +499,14 @@ task:
 
     # Use larger ccache cache, as this task compiles with multiple compilers /
     # flag combinations
-    CCACHE_MAXSIZE: "1GB"
+    CCACHE_MAXSIZE: "3G"
     CCACHE_DIR: "/tmp/ccache_dir"
 
     LINUX_CONFIGURE_FEATURES: *LINUX_CONFIGURE_FEATURES
     LINUX_MESON_FEATURES: *LINUX_MESON_FEATURES
 
   # task that did not run, count as a success, so we need to recheck Linux'
-  # condition here ...
+  # condition here; cirus warns if the "only_if" condition doesn't match the task being depended on
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*linux.*'
 
   container:
-- 
2.25.1


--rPFbv4B2w2tcwGo5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0008-cirrus-warnings-use-a-single-common-always-block.patch"



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 12:00  Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 5+ messages in thread

From: Andrew Dunstan @ 2024-05-07 12:00 UTC (permalink / raw)
  To: Richard Guo <[email protected]>; David Rowley <[email protected]>; +Cc: jian he <[email protected]>; PostgreSQL Hackers <[email protected]>


On 2024-05-07 Tu 06:05, Richard Guo wrote:
>
> On Tue, May 7, 2024 at 1:22 PM David Rowley <[email protected]> wrote:
>
>     It would be good to get some build farm coverage of this so we don't
>     have to rely on manual testing.  I wonder if it's a good idea to just
>     define REALLOCATE_BITMAPSETS when #ifdef CLOBBER_FREED_MEMORY... or if
>     we should ask on the buildfarm-members list if someone wouldn't mind
>     defining it?
>
>
> +1 to have build farm coverage of REALLOCATE_BITMAPSETS. This flag
> seems quite useful.
>
>

I have added it to the CPPFLAGS on prion.


cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com


^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 18:20  Tom Lane <[email protected]>
  parent: Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 5+ messages in thread

From: Tom Lane @ 2024-05-07 18:20 UTC (permalink / raw)
  To: Andrew Dunstan <[email protected]>; +Cc: Richard Guo <[email protected]>; David Rowley <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>

Andrew Dunstan <[email protected]> writes:
> On 2024-05-07 Tu 06:05, Richard Guo wrote:
>> +1 to have build farm coverage of REALLOCATE_BITMAPSETS. This flag
>> seems quite useful.

> I have added it to the CPPFLAGS on prion.

... and as expected, prion fell over.

I find that Richard's proposed fix makes the core regression tests
pass, but we still fail check-world.  So I'm afraid we need something
more aggressive, like the attached which makes make_restrictinfo
copy all its input bitmapsets.  Without that, we still have sharing
of bitmapsets across different RestrictInfos, which seems pretty
scary given what we now see about the effects of 00b41463c.  This
seems annoyingly expensive, but maybe there's little choice?

Given this, we could remove ad-hoc bms_copy calls from the callers
of make_restrictinfo, distribute_quals_to_rels, etc.  I didn't go
looking for possible wins of that sort; there's unlikely to be a
lot of them.

			regards, tom lane



Attachments:

  [text/x-diff] copy-bitmapsets-when-making-restrictinfos.patch (1.1K, ../../[email protected]/2-copy-bitmapsets-when-making-restrictinfos.patch)
  download | inline diff:
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index 0b406e9334..e81861bc8b 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -132,8 +132,8 @@ make_restrictinfo_internal(PlannerInfo *root,
 	restrictinfo->is_clone = is_clone;
 	restrictinfo->can_join = false; /* may get set below */
 	restrictinfo->security_level = security_level;
-	restrictinfo->incompatible_relids = incompatible_relids;
-	restrictinfo->outer_relids = outer_relids;
+	restrictinfo->incompatible_relids = bms_copy(incompatible_relids);
+	restrictinfo->outer_relids = bms_copy(outer_relids);
 
 	/*
 	 * If it's potentially delayable by lower-level security quals, figure out
@@ -191,7 +191,7 @@ make_restrictinfo_internal(PlannerInfo *root,
 
 	/* required_relids defaults to clause_relids */
 	if (required_relids != NULL)
-		restrictinfo->required_relids = required_relids;
+		restrictinfo->required_relids = bms_copy(required_relids);
 	else
 		restrictinfo->required_relids = restrictinfo->clause_relids;
 


^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 22:35  David Rowley <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 5+ messages in thread

From: David Rowley @ 2024-05-07 22:35 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>

On Wed, 8 May 2024 at 06:20, Tom Lane <[email protected]> wrote:
> I find that Richard's proposed fix makes the core regression tests
> pass, but we still fail check-world.  So I'm afraid we need something
> more aggressive, like the attached which makes make_restrictinfo
> copy all its input bitmapsets.  Without that, we still have sharing
> of bitmapsets across different RestrictInfos, which seems pretty
> scary given what we now see about the effects of 00b41463c.  This
> seems annoyingly expensive, but maybe there's little choice?

We could make the policy copy-on-modify.  If you put bms_copy around
the bms_del_member() calls in remove_rel_from_query(), does it pass
then?

David






^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 22:37  David Rowley <[email protected]>
  parent: David Rowley <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: David Rowley @ 2024-05-07 22:37 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>

On Wed, 8 May 2024 at 10:35, David Rowley <[email protected]> wrote:
>
> On Wed, 8 May 2024 at 06:20, Tom Lane <[email protected]> wrote:
> > I find that Richard's proposed fix makes the core regression tests
> > pass, but we still fail check-world.  So I'm afraid we need something
> > more aggressive, like the attached which makes make_restrictinfo
> > copy all its input bitmapsets.  Without that, we still have sharing
> > of bitmapsets across different RestrictInfos, which seems pretty
> > scary given what we now see about the effects of 00b41463c.  This
> > seems annoyingly expensive, but maybe there's little choice?
>
> We could make the policy copy-on-modify.  If you put bms_copy around
> the bms_del_member() calls in remove_rel_from_query(), does it pass
> then?

err, I mean bms_copy() the set before passing to bms_del_member().

David






^ permalink  raw  reply  [nested|flat] 5+ messages in thread


end of thread, other threads:[~2024-05-07 22:37 UTC | newest]

Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-03 05:10 [PATCH 7/8] cirrus/ccache: disable compression and show stats Justin Pryzby <[email protected]>
2024-05-07 12:00 Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Andrew Dunstan <[email protected]>
2024-05-07 18:20 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-07 22:35   ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. David Rowley <[email protected]>
2024-05-07 22:37     ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. David Rowley <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox