Improved png memory I/O performance. #241

tdhintz · 2017-04-07T10:54:42Z

This primarily benefits Tesseract OCR on the Windows platform because it replaces file I/O with memory operations.

amitdo · 2017-04-07T11:29:14Z

src/memio.h

+#ifndef  LEPTONICA_MEMIO_H
+#define  LEPTONICA_MEMIO_H
+
+#include <png.h>


You should add here:

#ifdef HAVE_CONFIG_H` #include "config_auto.h" #endif /* HAVE_CONFIG_H */

and wrap the rest of the code in:

/* --------------------------------------------*/ #if HAVE_LIBPNG /* --------------------------------------------*/ #endif /* HAVE_LIBPNG */ /* --------------------------------------------*/

https://github.com/DanBloomberg/leptonica/blob/8a839a254e8d2e/src/pngio.c#L113

amitdo · 2017-04-07T11:42:13Z

The Travis build failed. There are compilation errors.
https://travis-ci.org/DanBloomberg/leptonica/builds/219636596

EDIT:
#242 has this comment

This builds in Visual Studio 2017, but not in the continuous integration environment for this project. Someone with experience on those platforms will need to correct the pull requests if you want the feature.

amitdo · 2017-04-07T12:04:34Z

src/memio.c

+* <pre>
+* libpng read/write callback replacements for performing memory I/O.
+* </pre>
+*/#include <string.h>


First, put a linebreak between the /* and the includes.

Second, in the rest of the codebase #include "allheaders.h" comes after the cstdlib headers:
https://github.com/DanBloomberg/leptonica/blob/8e7190ccfb46c7/src/pix4.c

tdhintz · 2017-04-07T12:23:36Z

To be clear, someone with experience with the build environment needs to make all of these changes.

amitdo · 2017-04-07T12:31:59Z

This PR is supposed to solve this Tesseract issue:
tesseract-ocr/tesseract#522

tdhintz · 2017-04-07T23:13:05Z

With this change I'm seeing about a 8 - 25% performance boost in Tesseract 4.0.0 LSTM w/OpenMP when passing a bi-tonal bitmap on Windows x64. Visual Studio 2017 was used for the builds. The higher number occurs when concurrency is used to process multiple pages at the same time. The lower number is experienced when performing 1 page OCR and no concurrency. I assume your performance will depend significantly on the number of png images generated as artifacts of the OCR process. I predict the more artifacts that are generated the greater the savings.

I notice that the code may be leaking the contents of "state" in error conditions.

DanBloomberg · 2017-04-07T23:49:13Z

8% improvement of the overall time on a single page seems surprising to me.

Can you break this down in terms of read time (both directly to memory and via file) and processing time,for a typical page of text at some stated resolution?

tdhintz · 2017-04-08T00:26:00Z

I understand. I posted because I'm interested in the results of others. I'm running small samples so a little variation can skew a lot. I have here a single page test that just averaged 13.08 over 3 runs without the change, and 3 runs with the change averaged 12.61 seconds. This is a gross measurement of overall time using the Weld .Net wrapper and iterating characters. 4% if I did my math correctly for this sample. Not 8% on that run but consistently measurable.

I haven't isolated the measurements into a test harness so I can't break it down by where the time is gained. I seems logical that there should be some gain on Windows given the old implementation. I see leptonica tries to give Windows a hint that the file doesn't actually need to be written but clearly there must be overhead using the temp directory even if the file isn't flushed (directory reads, security checks, file name generation, etc).

On systems with fmemopen() I'd have to guess that the two implementations are close to a wash but I have no way of measuring that.

DanBloomberg · 2017-04-08T00:46:31Z

No question in my mind that for reading and writing via memory, doing it directly is better than via a temporary file and using a file stream interface -- even if the latter is done securely using tmpfile() or similar methods. This should ultimately be enabled for all platforms.

Do you have any idea why Microsoft hasn't yet implemented the fmemopen and openmemstream() library functions, which are POSIX 2008? How hard could this be...

tdhintz · 2017-04-08T01:06:55Z

I don't have special insight, but Microsoft hasn't historically had a POSIX focus. Maybe their new cross platform strategies will lead them there.

stweil · 2017-04-17T20:09:54Z

src/pngio.c

+	png_textp    text_ptr;  /* ptr to text_chunk */
+	PIX         *pix, *pixt;
+	PIXCMAP     *cmap;
+	struct MemIOData state;


I suggest to remote struct here and simply use MemIOData.

stweil · 2017-04-17T20:12:59Z

src/memio.h

+	/*
+	* <pre>Pointer to the next node in the list.  Zero if this is the last node.</pre>
+	*/
+	MemIOData* m_Next;


Here struct is needed because MemIOData without struct is still undefined.

stweil · 2017-04-17T20:13:39Z

src/memio.h

+	* <pre>Pointer to the last node in the linked list.  The last node is where new
+	* content is written.</pre>
+	*/
+	MemIOData* m_Last;


Here struct is needed, see above.

stweil · 2017-04-17T20:14:22Z

src/pngio.c

+	PIX         *pixt;
+	PIXCMAP     *cmap;
+	char        *text;
+	struct MemIOData state;


Remove struct here.

stweil · 2017-04-17T20:18:22Z

src/memio.c

@@ -0,0 +1,181 @@
+/*====================================================================*
+-  Copyright (C) 2017 Milner Technologies, Inc.  This content is a
+-  component of leptonica and is provided under the terms of the


I suggest to use the upper case form Leptonica here and also in the next line.

stweil · 2017-04-17T20:22:39Z

The Travis build failed.

Those two build errors are caused by missing struct keywords (see my inline comments).
Obviously the MS compiler tolerates non standard C code whereas gcc raises errors.

DanBloomberg · 2017-04-18T01:29:19Z

Note also that the code must be strict ANSI C.

No 'new', 'delete'
all variable declarations immediately following an open brace

And there are some strict guidelines, in particular no asserts (library cannot exit)

See also style-guide.txt in the root directory, mostly for formatting.

stweil · 2017-04-18T04:51:18Z

Note also that the code must be strict ANSI C.

@tdhintz, there are compiler options for Visual C which disable the Microsoft extensions. Try /Za, maybe in combination with /Zc or other options.

tdhintz · 2017-04-18T17:32:05Z

The project used C++. I don't recall whether I did that or the cmake. I can't think in C so the prototype was C++ and I translated it back to C. Performance with the new vs malloc() was better in a brief test so I left it that way for safety. It is most probable that the difference was due to some factor other than the use of new, but I stuck with the lucky hat. All the suggestions on code format and C compatibility have been diverting but the better question is whether the code improves performance on Win platform. If it doesn't then this exercise has no benefit.

DanBloomberg · 2017-04-18T19:45:57Z

You mentioned that the memio buffers can have a memory leak with a low-level failure (e.g., causing setjmp). What is the current situation?

tdhintz · 2017-04-18T19:47:58Z

The current commit handles memory correctly on error conditions.

DanBloomberg · 2017-04-20T20:00:18Z

Let me take a day or two to verify that this will work OK with 'make allheaders', which generates allheaders.h, and that it passes all the regression tests for png I/O -- and I'll merge it as is.

After that, I'll need to incorporate memio.{h,c} directly into pngio.c to make its functions static, so that they do not get scooped up into allheaders.h by xtractprotos. And I'll be able to rewrite the now-redundant Stream functions as thin shims of your Mem functions.

DanBloomberg · 2017-04-24T17:23:10Z

Slightly modified version is in and working.

As it turned out, I couldn't easily make the pix read/write stream functions as shims of the mem functions, because the pixa read/write functions are now using the stream interface for each individual pix, and the mem functions are not designed to work consecutively. It's possible to handle this in pixaReadMem(), for example, but it's not clear that it's worth the effort.

Improvide png memory I/O performance.

aa21bcc

amitdo reviewed Apr 7, 2017

View reviewed changes

tdhintz changed the title ~~Improvide png memory I/O performance.~~ Improved png memory I/O performance. Apr 7, 2017

stweil requested changes Apr 17, 2017

View reviewed changes

stweil mentioned this pull request Apr 17, 2017

Pull request #241, Tesseract performance tune, doesn't build #242

Closed

tdhintz added 3 commits April 18, 2017 11:06

Update memio.c

2b98ea6

Update memio.h

9fd8be3

Update pngio.c

cb3a63d

DanBloomberg merged commit aae8bc3 into DanBloomberg:master Apr 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved png memory I/O performance. #241

Improved png memory I/O performance. #241

tdhintz commented Apr 7, 2017

amitdo Apr 7, 2017 •

edited

Loading

amitdo commented Apr 7, 2017 •

edited

Loading

amitdo Apr 7, 2017 •

edited

Loading

tdhintz commented Apr 7, 2017 via email •

edited

Loading

amitdo commented Apr 7, 2017

tdhintz commented Apr 7, 2017

DanBloomberg commented Apr 7, 2017

tdhintz commented Apr 8, 2017

DanBloomberg commented Apr 8, 2017 •

edited

Loading

tdhintz commented Apr 8, 2017

stweil Apr 17, 2017

stweil Apr 17, 2017

stweil Apr 17, 2017

stweil Apr 17, 2017

stweil Apr 17, 2017

stweil commented Apr 17, 2017

DanBloomberg commented Apr 18, 2017

stweil commented Apr 18, 2017 •

edited

Loading

tdhintz commented Apr 18, 2017

DanBloomberg commented Apr 18, 2017

tdhintz commented Apr 18, 2017

DanBloomberg commented Apr 20, 2017

DanBloomberg commented Apr 24, 2017

Improved png memory I/O performance. #241

Improved png memory I/O performance. #241

Conversation

tdhintz commented Apr 7, 2017

amitdo Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

amitdo commented Apr 7, 2017 • edited Loading

amitdo Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

tdhintz commented Apr 7, 2017 via email • edited Loading

amitdo commented Apr 7, 2017

tdhintz commented Apr 7, 2017

DanBloomberg commented Apr 7, 2017

tdhintz commented Apr 8, 2017

DanBloomberg commented Apr 8, 2017 • edited Loading

tdhintz commented Apr 8, 2017

stweil Apr 17, 2017

Choose a reason for hiding this comment

stweil Apr 17, 2017

Choose a reason for hiding this comment

stweil Apr 17, 2017

Choose a reason for hiding this comment

stweil Apr 17, 2017

Choose a reason for hiding this comment

stweil Apr 17, 2017

Choose a reason for hiding this comment

stweil commented Apr 17, 2017

DanBloomberg commented Apr 18, 2017

stweil commented Apr 18, 2017 • edited Loading

tdhintz commented Apr 18, 2017

DanBloomberg commented Apr 18, 2017

tdhintz commented Apr 18, 2017

DanBloomberg commented Apr 20, 2017

DanBloomberg commented Apr 24, 2017

amitdo Apr 7, 2017 •

edited

Loading

amitdo commented Apr 7, 2017 •

edited

Loading

amitdo Apr 7, 2017 •

edited

Loading

tdhintz commented Apr 7, 2017 via email •

edited

Loading

DanBloomberg commented Apr 8, 2017 •

edited

Loading

stweil commented Apr 18, 2017 •

edited

Loading