Add page cache option to save memory #474

ssigwart · 2022-01-25T15:37:22Z

Running example_067.php with and without the cache enabled uses about 26MB without vs 17MB with the cache.

This adds the usePageCacheFile option which allow you to move page data to a temporary file when not in use.

examples/example_067.php

+// Begin       : 2022-01-25
+// Last Update : 2022-01-25
+//
+// Description : Example 066 for TCPDF class


williamdes · 2022-01-25T16:04:28Z

examples/index.php

@@ -85,6 +85,7 @@
 <li>Text stretching and spacing (tracking/kerning): [<a href="example_063.php" title="PDF [new window]" target="_blank">PDF</a>]</li>
 <li>No-write page regions: [<a href="example_064.php" title="PDF [new window]" target="_blank">PDF</a>]</li>
 <li>PDF/A-1b (ISO 19005-1:2005) document: [<a href="example_065.php" title="PDF [new window]" target="_blank">PDF</a>]</li>
+<li>Using page cache to reduce memory usage: [<a href="example_066.php" title="PDF [new window]" target="_blank">PDF</a>]</li>


Suggested change

<li>Using page cache to reduce memory usage: [<a href="example_066.php" title="PDF [new window]" target="_blank">PDF</a>]</li>

<li>Using page cache to reduce memory usage: [<a href="example_067.php" title="PDF [new window]" target="_blank">PDF</a>]</li>

williamdes · 2022-01-25T16:07:12Z

tcpdf.php

+		if ($this->pageCacheFile === false)
+			$this->pageCacheFile = null;
+		else
+			self::$pageCacheRefCnts[(int)$this->pageCacheFile] = 1;


Suggested change

if ($this->pageCacheFile === false)

$this->pageCacheFile = null;

else

self::$pageCacheRefCnts[(int)$this->pageCacheFile] = 1;

if ($this->pageCacheFile === false) {

$this->pageCacheFile = null;

} else {

self::$pageCacheRefCnts[(int)$this->pageCacheFile] = 1;

}

williamdes · 2022-01-25T16:07:31Z

tcpdf.php

+		if ($this->pageCacheFile !== null)
+		{
+			self::$pageCacheRefCnts[(int)$this->pageCacheFile]--;
+			if (self::$pageCacheRefCnts[(int)$this->pageCacheFile] == 0)


please use { for your if blocks

ssigwart · 2022-01-25T16:11:51Z

Thanks for reviewing it, @williamdes! I think I got all the changes in and updated all the ifs.

williamdes · 2022-01-27T23:21:24Z

tcpdf.php

+		{
+			self::$pageCacheRefCnts[(int)$this->pageCacheFile]--;
+			if (self::$pageCacheRefCnts[(int)$this->pageCacheFile] == 0) {
+				@fclose($this->pageCacheFile); // Suppress error since this might be the end of the PHP script


A is ressource check might remove the need of silencing an error

Good suggestion. I ran a test and that seems like a better way to do this. I updated it.

williamdes · 2022-01-27T23:21:57Z

tcpdf.php

+		}
+	}
+
+	/** Handle cloning */


Why is this needed, it feels a bit incomplete or non explained

I updated the comment. It's so that if you clone the object, you don't have two objects writing (and overwriting each other) to the same file.

williamdes · 2022-01-27T23:23:37Z

tcpdf.php

+							self::$pageCacheRefCnts[(int)$this->pageCacheFile] = 1;
+							// Copy data
+							fseek($origPageCacheFile, 0, SEEK_SET);
+							while (($copyContent = fread($origPageCacheFile, 8192)) != false)


This is missing {

Could you add a bit more comments on what each step does?

I added the brace and added comments.

williamdes · 2022-01-28T08:36:03Z

tcpdf.php

@@ -7866,6 +7891,60 @@ public function _destroy($destroyall=false, $preserve_objcopy=false) {
 		}
 	}

+	/** Page cache reference counts */
+	protected static $pageCacheRefCnts = [];


Suggested change

protected static $pageCacheRefCnts = [];

protected static $pageCacheRefCnts = array();

Old php compatibility

I updated this and one other place I used [].

williamdes · 2022-01-28T08:38:01Z

tcpdf.php

@@ -21201,7 +21332,9 @@ public function deletePage($page) {
 			return false;
 		}
 		// delete current page
-		unset($this->pages[$page]);
+		if (isset($this->pages[$page])) {
+			unset($this->pages[$page]);


A bit too much indentation
Same below

Sorry, I don't understand this comment. It seems to match the other if statements.

ssigwart · 2022-01-28T13:50:39Z

tcpdf.php

+
+	/**
+	 * Page cache index
+	 * @protected


I updated this.

williamdes

This code is crystal clear, thank you!

williamdes

💯

ssigwart · 2022-02-03T01:56:50Z

@williamdes, I assume someone else will merge this at some point, right? I don't seem to have permission.

williamdes · 2022-02-03T11:47:06Z

@williamdes, I assume someone else will merge this at some point, right? I don't seem to have permission.

Yes, when Nicolas will have some time he will probably review this :)
Now we need to be patient

ssigwart · 2022-02-03T13:05:38Z

Sounds good. Thank you.

williamdes · 2023-08-02T20:54:16Z

tcpdf.php

@@ -7866,6 +7892,60 @@ public function _destroy($destroyall=false, $preserve_objcopy=false) {
 		}
 	}

+	/** Page cache reference counts */
+	protected static $pageCacheRefCnts = array();


can this be non static ?

I intentionally have it static so that it can be shared when you clone it. The PR implements copy-on-write, so if you clone the PDF, it will share the cache until an edit is made.

I've been including this patch on production since 2019 and I haven't noticed any issues with it, so I feel like it's safer to leave this as-is, but if you want, I can remove the shared cache pages and make this non-static.

I think it could cause issues to users that do not expect things to be cached between pdfs

Do you mean people using the library? They would have to enable the page cache with usePageCacheFile(...) and even if they do, I have the code internally handling the caching between PDFs, so I don't think it would cause issues or be something users even know about.

If you are referring to people developing the library, that's where I think it might cause confusion, but I think I can handle that by writing more detailed comments.

If the static part is something you want to change, but still keep caching across PDFs, I could implement that with a new object for reference counting and since objects are shared by reference, it would act very similarly to the static variable.

I think that if you can implement a version with objects that would be better than a static variable. @MauricioFauth maybe you would have input on this feature and the use of statics?

I added e5a3a7ce85c5c18066fcff7f87c3cf3f25fca2ad, which does it with an object instead. I'm not as confident in this change since it hasn't had years of real world testing, but it seems correct. Personally, I think this might be a little more confusing for library developers. With the static variable, I think it's more obvious that it's used to share data than this object by reference approach.

Thanks, I hope someone with merge power can review this some time soon
The commit can be reverted if the static approach is preferred by the reviewer

…#620) Tested and confirmed working in PHP 7.4 and PHP 8.2

* README: tone down the warning about tc-lib-pdf Signed-off-by: Ruben Barkow-Kuder <github@r.z11.de> * Update README.md Co-authored-by: William Desportes <williamdes@wdes.fr> --------- Signed-off-by: Ruben Barkow-Kuder <github@r.z11.de> Co-authored-by: William Desportes <williamdes@wdes.fr>

* Fix of Deprecation warning with php 8.1 tecnickcom#614 * Update include/barcodes/qrcode.php Co-authored-by: William Desportes <williamdes@wdes.fr> --------- Co-authored-by: Robert Johnson Nallori <rnallori@evoketechnologies.com> Co-authored-by: johnson361 <johnson361@users.noreply.github.com> Co-authored-by: William Desportes <williamdes@wdes.fr>

…om#633) * Update tcpdf_fonts.php Fixes "use of "self" in callables is deprecated" warning is arising from tcpdf_fonts.php when using PHP >= 8.2 * Update tcpdf_fonts.php for PHP 5.3-8.2 compatibility PHP 8.2 "use of "self" in callables is deprecated" yet some ways of fixing this breaks for PHP 5.3. This approach works, tested PHP 5.3.29 - 8.2.0 * Update tcpdf_fonts.php Spaces added back in before arguments * Update tcpdf_fonts.php using get_called_class() Maneuvers compatibility of callables inside array_map() between PHP 5.3 and 8.2 - tested. --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

* Fix composite glyph output * Pad zeros before checksum calulation --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

* fix for tecnickcom#583 * fix fix --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

* Fix non-numeric value warning Fixes this warning on generating PDF: Warning: A non-numeric value encountered in /tcpdf/tcpdf.php on line 5473 * Better fix for non-numeric value warning Fixes this warning on generating PDF after calling `Text` with a non-numeric value for `$fstroke`: Warning: A non-numeric value encountered in /tcpdf/tcpdf.php on line 5470 * Update tcpdf.php Co-authored-by: William Desportes <williamdes@wdes.fr> --------- Co-authored-by: William Desportes <williamdes@wdes.fr> Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

This was already fixed in tc-lib-barcode.

* Fix return type annotation * BC with tools that do not support PHPStan annotations Co-authored-by: William Desportes <williamdes@wdes.fr> --------- Co-authored-by: William Desportes <williamdes@wdes.fr> Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

tecnickcom#598) * Typehints for get/setHeaderMargin are inconstent * Add typehints for header/footer margin properties --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

* Bump actions/checkout and add PHP 8.3 * Update the composer json tests * Remove PHP 5.3 and 5.4 from the matrix * add permission entry Restrict GitHub actions access --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

Running example_068.php with and without the cache enabled uses about 26MB without vs 17MB with the cache.

ssigwart · 2023-10-24T15:00:11Z

I rebased this to fix the merge conflicts with the examples.

codecov-commenter · 2023-10-24T15:17:33Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (5fce932) 69.28% compared to head (f186d90) 72.96%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #474      +/-   ##
==========================================
+ Coverage   69.28%   72.96%   +3.68%     
==========================================
  Files         128      129       +1     
  Lines       26961    21974    -4987     
==========================================
- Hits        18679    16034    -2645     
+ Misses       8282     5940    -2342

Flag	Coverage Δ
php-5.3-ubuntu-latest	`?`
php-5.4-ubuntu-latest	`?`
php-5.5-ubuntu-latest	`?`
php-5.6-ubuntu-latest	`?`
php-7.0-ubuntu-latest	`?`
php-8.3-ubuntu-latest	`72.96% <84.53%> (?)`
php-nightly-ubuntu-latest	`72.96% <84.53%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
examples/example_068.php	`100.00% <100.00%> (ø)`
include/tcpdf_page_cache_reference_counts.php	`100.00% <100.00%> (ø)`
tcpdf.php	`71.69% <70.58%> (+3.77%)`	⬆️

... and 81 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ssigwart · 2023-10-24T16:04:29Z

I also updated this so it passes the unit tests, including on PHP 5.3.

williamdes · 2023-12-20T10:35:05Z

I also updated this so it passes the unit tests, including on PHP 5.3.

Thank you !
We will have to wait on @nicolaasuni to a review

nicolaasuni · 2024-10-26T10:05:50Z

While this is a nice feature to have, I am not keen to accept new features as the tc-lib-pdf is already working and missing only (even if very important) HTML/CSS/SVG/Javascript support.

ssigwart · 2024-10-26T12:44:30Z

Thanks, @nicolaasuni. That makes sense. I'm looking forward to the production release of tc-lib-pdf to see how it works with some of the large CSVs I generate.

ssigwart force-pushed the pageCache branch 2 times, most recently from 89671d1 to 634d7ee Compare January 25, 2022 15:44

williamdes reviewed Jan 25, 2022

View reviewed changes

examples/example_067.php Outdated

// Begin : 2022-01-25

// Last Update : 2022-01-25

//

// Description : Example 066 for TCPDF class

This comment was marked as resolved.

Sign in to view

williamdes reviewed Jan 25, 2022

View reviewed changes

williamdes reviewed Jan 27, 2022

View reviewed changes

williamdes reviewed Jan 28, 2022

View reviewed changes

tcpdf.php

/**

* Page cache index

* @protected

This comment was marked as resolved.

Sign in to view

Copy link

Contributor Author

ssigwart Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this.

williamdes previously approved these changes Jan 28, 2022

View reviewed changes

ssigwart dismissed williamdes’s stale review via 7cf9c9f January 28, 2022 13:49

williamdes approved these changes Jan 28, 2022

View reviewed changes

williamdes reviewed Aug 2, 2023

View reviewed changes

remicollet and others added 10 commits September 6, 2023 08:51

Fix SPDX license ID (tecnickcom#591)

f7b000d

Applied fix found by BrainforgeUK in Issue tecnickcom#564 (tecnickcom…

70b63fa

…#620) Tested and confirmed working in PHP 7.4 and PHP 8.2

php 8+ edge cases fixes (tecnickcom#630)

de332db

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

Fix composite glyph output (tecnickcom#581)

7e3fb6f

* Fix composite glyph output * Pad zeros before checksum calulation --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

fix for tecnickcom#583 (tecnickcom#584)

9c7982d

* fix for tecnickcom#583 * fix fix --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

Fixed problems with S25 barcode (tecnickcom#611)

26d2eed

This was already fixed in tc-lib-barcode.

mvorisek and others added 17 commits September 6, 2023 09:29

Check fonts files with phpstan too (tecnickcom#575)

548c0f2

Typehints for getHeaderMargin() and setHeaderMargin() are inconsistent (

18d1b2b

tecnickcom#598) * Typehints for get/setHeaderMargin are inconstent * Add typehints for header/footer margin properties --------- Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>

Bump version

f209e39

Update copyright year

96985b9

Update the CHANGELOG for 6.6.3 (tecnickcom#643)

e3a8409

Bump version

31b9389

Fix corrupted file

3ca051c

Simplify test matrix

037283c

Remove duplicate settings

7689851

Adjust automation test settings

5fce932

Add page cache option to save memory

178eb82

Running example_068.php with and without the cache enabled uses about 26MB without vs 17MB with the cache.

Review fixes

3a7d97f

More review updates

fb79d56

Array updates

df109d8

Swap static variable for shared reference to object

91eec42

ssigwart force-pushed the pageCache branch from e5a3a7c to 91eec42 Compare October 24, 2023 14:59

Fix unit test failure

f186d90

ssigwart added 2 commits October 24, 2023 11:25

Page cache PHP 5.3 support

38b38a9

Page cache PHP 5.3 support in tests

677f53f

nicolaasuni force-pushed the main branch from f9fd218 to 82fc97b Compare March 25, 2024 23:53

nicolaasuni closed this Oct 26, 2024

nicolaasuni added the wontfix label Oct 26, 2024

	<li>Using page cache to reduce memory usage: [<a href="example_066.php" title="PDF [new window]" target="_blank">PDF</a>]</li>
	<li>Using page cache to reduce memory usage: [<a href="example_067.php" title="PDF [new window]" target="_blank">PDF</a>]</li>

	protected static $pageCacheRefCnts = [];
	protected static $pageCacheRefCnts = array();

Add page cache option to save memory #474

Add page cache option to save memory #474

Conversation

ssigwart commented Jan 25, 2022 • edited Loading

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssigwart commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

williamdes left a comment

Choose a reason for hiding this comment

williamdes left a comment

Choose a reason for hiding this comment

ssigwart commented Feb 3, 2022

williamdes commented Feb 3, 2022

ssigwart commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssigwart commented Oct 24, 2023

codecov-commenter commented Oct 24, 2023

Codecov Report

ssigwart commented Oct 24, 2023

williamdes commented Dec 20, 2023

nicolaasuni commented Oct 26, 2024

ssigwart commented Oct 26, 2024

ssigwart commented Jan 25, 2022 •

edited

Loading