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

Fix auto size columns #1130

Merged
merged 18 commits into from
Aug 23, 2023
Merged

Fix auto size columns #1130

merged 18 commits into from
Aug 23, 2023

Conversation

Bykiev
Copy link
Collaborator

@Bykiev Bykiev commented Jul 26, 2023

This PR fixes #1022, but still not finished because SixLabors.Fonts returns incorrect default char width. This bug was already fixed in master branch of SixLabors.Fonts, PR will be updated after release

This PR fixes nissl-lab#1022, but still not finished because SixLabors.Fonts returns incorrect default chart width. This bug was already fixed in master branch of SixLabors.Fonts, PR will be updated after release
@Bykiev
Copy link
Collaborator Author

Bykiev commented Aug 11, 2023

After update SixLabors.Fonts it seems returns incorrect height

@Bykiev
Copy link
Collaborator Author

Bykiev commented Aug 12, 2023

@tonyqus, hello!

It seems I was wrong and SixLabors.Fonts now returns correct height for the fonts. I still don't understand how to get the height similar to Excel. For string "test" with 96 dpi monitor, 20pt Calibri font the calculated height is 26.25 pt. SixLabors.Fonts returns height 18px (with correction 1.1 the calculated heigh of row is 181.120 = 360, which almost twice lower than calculated by Excel. What do you think about it?

@Bykiev
Copy link
Collaborator Author

Bykiev commented Aug 12, 2023

Sorry, it's the best what I can do, don't know why does it fails on Ubuntu. Help is wanted

@Bykiev
Copy link
Collaborator Author

Bykiev commented Aug 16, 2023

Thought all tests are passed, but it's not like excel calculating sizes. It's the best what I can do, it seems nobody knows how excel calculates it, it's a heavy task.

@Bykiev Bykiev marked this pull request as ready for review August 16, 2023 19:33
@tonyqus
Copy link
Member

tonyqus commented Aug 19, 2023

#1164

SixLabors.Fonts released 1.0. Can you update the assembly and check if they fixed something on height calculation.

@tonyqus tonyqus added this to the NPOI 2.6.2 milestone Aug 19, 2023
@Bykiev
Copy link
Collaborator Author

Bykiev commented Aug 19, 2023

#1164

SixLabors.Fonts released 1.0. Can you update the assembly and check if they fixed something on height calculation.

Hello, this PR is already based on stable version of SixLabors.Fonts. There were fixed many errors and API has been changed, that's why old versions of NPOI are not compatible with stable version of SixLabors.Fonts. There is no more Measure method and instead of it these methods are available

MeasureAdvance - This measures the vertical and horizontal advance of the glyphs and text. I.E the glyph bounds + any padding. This includes line height.

MeasureSize - This measures the glyph bounds and the vertical ascent which includes any leading vertical padding.

MeasureBounds - This measures the glyph bounds with no padding.

Thought I have changed some tests it seems they were not accurate. Please review PR carefully

@tonyqus
Copy link
Member

tonyqus commented Aug 19, 2023

Sorry, it's my fault. In the last review, I didn't see csproj is updated to use 1.0.

I'm eager to update this assembly since it can also resolve nuget installation issue #964. It somewhat becomes a obstacle of making 2.6.0 the most downloaded version. (Currently, 2.5.6 is the most popular one).

@tonyqus tonyqus changed the base branch from master to release2.6.2 August 23, 2023 07:42
@tonyqus tonyqus changed the base branch from release2.6.2 to master August 23, 2023 07:42
@tonyqus tonyqus modified the milestones: NPOI 2.6.2, NPOI 2.7.0 Aug 23, 2023
@tonyqus
Copy link
Member

tonyqus commented Aug 23, 2023

LGTM

@tonyqus tonyqus merged commit 14252d5 into nissl-lab:master Aug 23, 2023
3 checks passed
@Sappharad
Copy link
Contributor

Just a heads-up, this appears to have resulted in significantly worse performance of HSSFSheet.AutoSizeColumn() in NPOI 2.6.2rc1. I'm seeing at least 6x as long to auto-size columns in a spreadsheet featuring around 1000 rows.

I realize it's probably out of your control because ImageSharp is doing the measurements for you, but I wanted to have it noted somewhere that it got a lot slower.

@Bykiev
Copy link
Collaborator Author

Bykiev commented Sep 5, 2023

Just a heads-up, this appears to have resulted in significantly worse performance of HSSFSheet.AutoSizeColumn() in NPOI 2.6.2rc1. I'm seeing at least 6x as long to auto-size columns in a spreadsheet featuring around 1000 rows.

I realize it's probably out of your control because ImageSharp is doing the measurements for you, but I wanted to have it noted somewhere that it got a lot slower.

Can you please share your test results? Do you have any thoughts how it can be improved?
Related issue SixLabors/Fonts#268

What about the accurate of autosizing?

@Bykiev
Copy link
Collaborator Author

Bykiev commented Sep 5, 2023

I did some tests and with SixLabors.Fonts it's 40-60x slower than System.Drawing.

From ClosedXML: ClosedXML/ClosedXML#1991

The problem here is

  • Unlike deprecated System.Drawing.Common, SixLabors.Fonts has to load fonts for each process. System.Drawing was calling native GDI/GDI+ through p/Invoke and all fonts were already in memory, ready to be used. That is the startup cost in first test.
  • SixLabors.Fonts TextMeasurer uses proper font shaping engine. Shaping engine is the thing that correctly deduces where each glyph should be to determine final position.

@Sappharad
Copy link
Contributor

Sappharad commented Sep 5, 2023

I did some tests and with SixLabors.Fonts it's 40-60x slower than System.Drawing.

I've also been doing some testing, since the test where I was seeing the performance regression was part of a large project. That test I was seeing 57 seconds to generate roughly 1000 rows and 5 columns of styled and formatted data with a header and footer and other things, compared to under 5 seconds before. The Visual Studio performance analyzer told me 97% of the time was spent in AutoSizeColumn() which is why I commented on this PR.

To provide a test that I could share, I made a stand-alone project that just generates 1000 rows and 5 columns and resizes them, I was seeing around 3 seconds in the new code. Still trying to figure out what's so complex about the bigger project test that makes it take so much longer, perhaps it's just that it's still .NET Framework and my test project is .NET 6.

It appears we were running NPOI 2.5.5 in the project in question prior to me upgrading to the latest which must still be using the System.Drawing code, as it takes 0.14 seconds in my test project.

In short, it seems like this problem was not really introduced here, but rather when measuring was switched to ImageSharp. I'm not sure if you want to pursue this any further or just leave it alone, like I said initially it was just a heads-up that it was slower. Sorry for the false alarm.

Here is some benchmark code I quickly threw together:

using NPOI.XSSF.UserModel;
using NPOI.SS.UserModel;
using System.Diagnostics;

string[] lorem = @"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut 
labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip 
ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat 
nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id 
est laborum.".Split(new char[] { ' ', '\r', '\n' });
int ipsum = 0;

using (IWorkbook workbook = new XSSFWorkbook())
{
    ISheet sheet1 = workbook.CreateSheet("Sheet1");

    Stopwatch sw = Stopwatch.StartNew();
    for (int rowNum = 1; rowNum <= 1000; rowNum++)
    {
        var row = sheet1.CreateRow(rowNum);
        for (int col = 1; col <= 5; col++)
        {
            row.CreateCell(col).SetCellValue(lorem[(ipsum++) % lorem.Length]);
        }
    }
    sw.Stop();
    Console.WriteLine($"Created rows in {sw.Elapsed}");
    sw.Restart();
    for (int col = 1; col <= 5; col++)
    {
        sheet1.AutoSizeColumn(col);
    }
    sw.Stop();
    Console.WriteLine($"Resized Columns in {sw.Elapsed}");
}

@lahma
Copy link
Collaborator

lahma commented Sep 6, 2023

Created #1179 to improve AutoSizeColumn performance (and probably others) a bit.

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

Successfully merging this pull request may close these issues.

Auto Size Column Not Working In 2.6.0
4 participants