Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hunspell 1.4.1 much slower than 1.3.3? #406

Closed
Fjan opened this issue Sep 17, 2016 · 16 comments
Closed

Hunspell 1.4.1 much slower than 1.3.3? #406

Fjan opened this issue Sep 17, 2016 · 16 comments
Milestone

Comments

@Fjan
Copy link

Fjan commented Sep 17, 2016

I just upgraded to the latest hunspell version (through brew) and noticed a slowdown of about 500%-800%, measured on several large directories with HTML pages. The slow down is consistent across different languages. Downgrading fixed the problem. Is there something wrong with the latest version as supplied through brew? How do I go about finding the cause of this?

@isotoxin
Copy link

Yes, 1.4.1 was refactored to use std::string, but std::string can be much slower, then raw char* buffer, especially if programmer don't understand what he doing.
PS. Im not hunspell contributor.

@PanderMusubi
Copy link
Contributor

Possible to rerefactor?

@dimztimz
Copy link
Contributor

dimztimz commented Oct 5, 2016

I just noticed that suggestions in Firefox are slower. I checked, and the 1.4 branch has landed in Firefox.

One possible reason may be that many fixed length buffers were replaced with vectors and strings. Those things can do a lot of dynamic allocation.

The plan from my standpoint is this:

  1. Document the code as Documenting the code #409
  2. Start doing major refactoring or even rewrite.
  3. For performance checks, profiling can be done. But I'd do that at only later stages.

ATM I can not start working on this, I have something else. So I'm just commenting right now :).

@dimztimz dimztimz modified the milestone: v2.0 Oct 26, 2016
@rvandermeulen
Copy link

FWIW, we've gotten a similar bug filed against Firefox now as well.
https://bugzilla.mozilla.org/show_bug.cgi?id=1322666

I can probably get some performance profiles if it helps, but I feel we're kind of stuck in a bad place here because of the number of code quality fixes that also shipped in 1.4.

@dimztimz
Copy link
Contributor

Profiling indeed can help, but it is questionable how much. Can we regain the speed of 1.3? Can we go faster, or maybe a little faster but not as 1.3. If you do have time, please do them and please document the used tools and the CXXFLAGS you put on make.

On the other hand, I already started writing v2.0, a complete rewrite, (see folder src/hunspell2) and I am very certain that if I finish it, it will be faster than any other previous version.

@rvandermeulen
Copy link

On the Firefox end of things, I bisected this down to 3bb456b. I could see that being a similar problem for the use case in the first comment here.

@dimztimz
Copy link
Contributor

The patch #450 slightly improves the speed but still the speed is not as great as in 1.3.3.

I guess I should bisected between 1.3.3 and 1.4.1 to really get to the real issue.

@dimztimz
Copy link
Contributor

This is the commit that introduces perfomance regression.

c1377e5 is the first bad commit
commit c1377e5
Author: Caolán McNamara
Date: Thu Dec 4 10:04:14 2014 +0000

coverity#17891 Copy into fixed size buffer

:040000 040000 6316699ade0cb0baf0a5e8ebb8fd71fa3ea65257 22f235493bb9272a4168e71582a331c0d2ac779b M src

@dimztimz
Copy link
Contributor

Ping @kaboomium see the above comment. The real performance regression is the commit above.

Can be solved either with short vector optimization (should get the same behavior as fixed length buffers), or even better, moving the dynamic allocations outside the hot loops.

@kaboomium
Copy link
Contributor

Hi Dimitrij,

The excessive calls to u8_u16() were what cachegrind blamed. With my change, a suggest test runs much faster and doesn't time-out for us any more. Perhaps head of tree is different from 1.4.1.

@Fjan
Copy link
Author

Fjan commented Dec 20, 2016

@kaboomium: Perhaps this issue, the 500%-800% slowdown on checking a directory of HTML files, is unrelated to the Firefox one. I got that result by doing a simple hunspell -l (list of misspelled words) without creating any suggestions.

@dimztimz
Copy link
Contributor

It is obvious that there are various performance regressions in 1.4. The ones in suggest() are very noticeable in GUI environment when you right click a misspeled word.

@Fjan can you tell me steps to reproduce and to measure your results?

@Fjan
Copy link
Author

Fjan commented Dec 20, 2016

@dimztimz I tried creating an easy to reproduce case for this comment but I discovered that the specific way my script works (by piping the files into hunspell one by one) has a large effect on the outcome.
First the basic case:

time hunspell -H -d ~/Library/Spelling/en_US -l <directory with ~100 HTML files>/**/*
# hunspell 1.3.3
user	0m0.131s
# hunspell 1.5.4
user	0m0.222s

Only 70% slower, not the 500% I see in my script. So next I tried piping one by one:

time for f in output/**/*; do cat $f | hunspell -H -d ~/Library/Spelling/en_US -l; done
# hunspell 1.3.3
user	0m2.030s
# hunspell 1.5.4
user	0m3.992s

About 97% slower, but still nowhere near the slowdown I see in my script and hunspell startup times contaminate the result. So this is the relevant excerpt from the Ruby script I used. It opens a separate thread and invokes hunspell once and pipes the files into it one by one.

# excerpt, some stuff removed for brevity
    files=Dir.glob('output/**/*')
    pipe=IO.popen(%{time hunspell -H -d #{dic} -p #{dic_loc}/hunspell_#{lang}.txt -l},'w+')
    p_in=Thread.new do
      files.each do |f|
        raw=::File.read(f)
         ...
        pipe.puts raw
      end
      pipe.close_write
    end
    p_out=Thread.new {output=pipe.readlines.map(&:chomp)}
    p_in.join;p_out.join
# hunspell 1.3.3
user	0m0.380s
# hunspell 1.5.4
user	0m4.205s

About 11 times slower... Im pretty confident that the difference is not in the ruby lines I removed from this script as those do not touch hunspell. Perhaps there is some kind of buffering issue related to pipes as it seems to "stutter" every dozen files or so?

@dimztimz
Copy link
Contributor

I just fixed ngsuggest() 5fc8e1f.

The problem is that the code is littered with bad use of dynamic allocations, especially with the vector<w_char> objects. The vector in the standard library does not have short vector optimization, the standard does not allow it. On the other hand the standard allows short string optimization.

Fixing the whole codebase is useless, the time taken would be the almost as the rewrite v2. My proposal is to fix only such hot methods (e.g. with lot of loops, quadratic algorithms) that give a noticeable performance drop. And the fixes should be done only by request, I won't start looking into the codebase and fix everything, don't have the time.

So far, I'm done with performance patches unless someone points to the exact commit that introduced a regression or points the method that makes trouble in a performance sense.

caolanm pushed a commit that referenced this issue Jan 23, 2017
kcachegrind reports 1,066,887,723 -> 894,015,631 on

echo Hollo | valgrind --tool=callgrind ./src/tools/.libs/hunspell -d nl_NL
@dimztimz
Copy link
Contributor

I guess this is fixed good enough in 1.6.1. Suggestions in 1.6.1 should even be a little faster than in 1.3.3. Detections maybe are slower bot no time to fix this in favor to v2.

@Fjan
Copy link
Author

Fjan commented Jul 24, 2017

I recently tested it again with 1.6.1, it's still 70%+ slower than 1.3.3 for regular spell checking without suggestions. (The large 500% slowdown seen in the script above were mainly due to the slower hunspell filling up the pipe buffer which caused it to stall momentarily.) Not sure if that's considered good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants