Skip to content

Commit

Permalink
Classify: Avoid unneeded new / delete operations
Browse files Browse the repository at this point in the history 
Both class variables BaselineCutoffs and CharNormCutoffs were pointers
to fixed size arrays which were allocated in the constructor and
deallocated in the destructor. These two extra allocations and two
extra deallocations can be avoided.

Signed-off-by: Stefan Weil <[email protected]>
  • Loading branch information
@stweil
stweil committed Apr 30, 2017
1 parent bb75793 commit 83588bc
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 7 deletions.
5 changes: 0 additions & 5 deletions classify/classify.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,18 +184,13 @@ Classify::Classify()
learn_debug_win_ = NULL;
learn_fragmented_word_debug_win_ = NULL;
learn_fragments_debug_win_ = NULL;

CharNormCutoffs = new uinT16[MAX_NUM_CLASSES];
BaselineCutoffs = new uinT16[MAX_NUM_CLASSES];
}

Classify::~Classify() {
EndAdaptiveClassifier();
delete learn_debug_win_;
delete learn_fragmented_word_debug_win_;
delete learn_fragments_debug_win_;
delete[] CharNormCutoffs;
delete[] BaselineCutoffs;
}


Expand Down
4 changes: 2 additions & 2 deletions classify/classify.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,8 @@ class Classify : public CCStruct {
// value in the adaptive classifier. Both are indexed by unichar_id.
// shapetable_cutoffs_ provides a similar value for each shape in the
// shape_table_
uinT16* CharNormCutoffs;
uinT16* BaselineCutoffs;
uinT16 CharNormCutoffs[MAX_NUM_CLASSES];
uinT16 BaselineCutoffs[MAX_NUM_CLASSES];
GenericVector<uinT16> shapetable_cutoffs_;
ScrollView* learn_debug_win_;
ScrollView* learn_fragmented_word_debug_win_;
Expand Down

11 comments on commit 83588bc

@theraysmith
Copy link
Contributor

Choose a reason for hiding this comment

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

This change won't compile at Google. The stack frame is too big. We have limits.

@amitdo
Copy link
Collaborator

@amitdo amitdo commented on 83588bc May 3, 2017

Choose a reason for hiding this comment

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

See also:
https://msdn.microsoft.com/en-us/library/7yhee2f0.aspx

@stweil
Copy link
Member Author

@stweil stweil commented on 83588bc May 4, 2017

Choose a reason for hiding this comment

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

Ray, do you know which variable is causing the "stack frame too big" problem? Could you please post the complete compiler error message?

@stweil
Copy link
Member Author

@stweil stweil commented on 83588bc May 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The GNU compiler can also warn when the stack frame exceeds a certain limit. Here is the result for Tesseract:

ccutil/params.cpp:89:1: warning: the frame size of 4144 bytes is larger than 4096 bytes [-Wframe-larger-than=]
viewer/scrollview.cpp:321:1: warning: the frame size of 4160 bytes is larger than 4096 bytes [-Wframe-larger-than=]
viewer/scrollview.cpp:409:1: warning: the frame size of 8240 bytes is larger than 4096 bytes [-Wframe-larger-than=]
viewer/scrollview.cpp:581:1: warning: the frame size of 8240 bytes is larger than 4096 bytes [-Wframe-larger-than=]
ccstruct/polyaprx.cpp:105:1: warning: the frame size of 12368 bytes is larger than 4096 bytes [-Wframe-larger-than=]
lstm/lstmtrainer.cpp:743:1: warning: the frame size of 4576 bytes is larger than 4096 bytes [-Wframe-larger-than=]
api/baseapi.cpp:997:1: warning: the frame size of 4208 bytes is larger than 4096 bytes [-Wframe-larger-than=]
api/pdfrenderer.cpp:846:1: warning: the frame size of 6336 bytes is larger than 4096 bytes [-Wframe-larger-than=]
api/pdfrenderer.cpp:931:1: warning: the frame size of 4272 bytes is larger than 4096 bytes [-Wframe-larger-than=]
training/fileio.cpp:160:1: warning: the frame size of 8224 bytes is larger than 4096 bytes [-Wframe-larger-than=]

The largest stack frame is caused here in file ccstruct/polyaprx.cpp. I currently don't see anything related to my change.

@amitdo
Copy link
Collaborator

@amitdo amitdo commented on 83588bc May 4, 2017

Choose a reason for hiding this comment

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

I believe they use clang.

http://clang-developers.42468.n3.nabble.com/clang-and-Wframe-larger-than-td4045414.html

@stweil
Copy link
Member Author

@stweil stweil commented on 83588bc May 4, 2017

Choose a reason for hiding this comment

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

The compiler option -Wframe-larger-than which is mentioned there for clang is also supported by gcc. I used it for the report shown above. As far as I remember, MS Visual C++ has a default limit of 1 MB for the frame size.

Ray, if the compile problem occurs with code outside of Tesseract, I suggest to use a pointer variable with new / delete instead of allocating it on the stack. This will still save one new / delete operations (3 before patch, 1 after patch with new / delete, 2 before patch, 0 after patch with variable on stack).

@amitdo
Copy link
Collaborator

@amitdo amitdo commented on 83588bc May 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Stefan, did you follow my 2 links above?

@stweil
Copy link
Member Author

@stweil stweil commented on 83588bc May 4, 2017

Choose a reason for hiding this comment

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

I did, but obviously I did not read the text on Visual C good enough. VC uses 16 KB, not 1 MB as the default frame size limit for warnings. Fixed now in my post above. The frame sizes of clang and gcc don't differ much: the largest frame size with clang is 12536 bytes.

@theraysmith
Copy link
Contributor

@theraysmith theraysmith commented on 83588bc May 4, 2017 via email

Choose a reason for hiding this comment

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

@stweil
Copy link
Member Author

@stweil stweil commented on 83588bc May 4, 2017

Choose a reason for hiding this comment

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

Yes, I know that my modification increased the size of Classify a lot. But I only found code which allocates that class on the heap (using new), no code which allocates it on the stack. The compiler (tested with gcc and clang) also did not find such code.

@stweil
Copy link
Member Author

@stweil stweil commented on 83588bc May 5, 2017

Choose a reason for hiding this comment

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

Now back to finding out why training isn't working properly...

Please have a look at pull request #883.

Please sign in to comment.