OfficeDev / office-js

A repo and NPM package for Office.js, corresponding to a copy of what gets published to the official "evergreen" Office.js CDN, at https://appsforoffice.microsoft.com/lib/1/hosted/office.js.
https://learn.microsoft.com/javascript/api/overview
Other
685 stars 95 forks source link

context.sync() takes more time to complete after clearing discontinuous cells in different sheets in Excel. #3566

Open jayakrishnankg opened 1 year ago

jayakrishnankg commented 1 year ago

I've the code to clear the discontinuous cells in different sheets in an Excel. The issue is that when I've one or more Excel workbooks opened in the background, the context.sync() call after clearing the cells in ClearContents function takes 2x longer compared to the previous iteration on the previous workbook. It keeps on growing 2x for every new workbook that's opened.

Environment

Following is the code snippet:

async Func1(){
  await Excel.run(async (context) => {
    await this.ClearCells(context);
  });
}

async ClearCells(context: Excel.RequestContext) {
 const RangeArray = new Array<Excel.Range>();
 for (let i = 0; i < namedRanges.length; i++) {
    const range = this.getRange(context, namedRanges[i]);
    RangeArray.push(range);
 }
 await this.ClearContents(context, RangeArray,true);     
}

getRange(context: Excel.RequestContext, cellNamedRange: string){
 try {
  const names = workbook.names;
  const name = names.getItemOrNullObject(cellNamedRange);
  const range = name.getRange();
  return range;
 } catch (e) {
  return null;
 }
}

async ClearContents(context: Excel.RequestContext, Ranges: Array<Excel.Range>,clearformatting:boolean) {
 if (Ranges == null || Ranges.length == 0)
  return;
 for (var i = 0; i < Ranges.length; i++) {
  Ranges[i].load({ formulas: true, address: true });
 }

 await context.sync();
 for (var i = 0; i < ranges.length; i++) {
  if (ranges[i] != null && (!isnullorundefined(ranges[i].formulas[0][0]) && ranges[i].formulas[0][0] == '')) {
    ranges[i].clear(excel.clearapplyto.contents);
    if (clearformatting)
      ranges[i].clear(excel.clearapplyto.formats);
    ranges[i].untrack();
  }
 }
 await context.sync(); //takes 2x times more compared to every last iteration 
}

I've also tried using RangeAreas to clear all at once, but it didn't make any difference in the execution time. I don't have any clue and stuck at the issue.

Expected behavior: context.sync() must take same time to execute even with the Excel worbooks in the background.

ghost commented 1 year ago

Thank you for letting us know about this issue. We will take a look shortly. Thanks.

jayakrishnankg commented 1 year ago

@donlvMSFT , any updates on this?

ZYUN-MSFT commented 1 year ago

Hi @jayakrishnankg ,

We are still investigating this issue. And we will update the status here soon. Thanks for your patient.

Thanks.

donlvMSFT commented 1 year ago

Hi @jayakrishnankg , thanks for the reporting.

I take some validation on my Mac environment but not see much difference when have background workbooks or not. So I want to confirm that, 1. Is this issue newly show up for you? If so, please share your version number. 2. Is your workbook with large data or any special contents?

Also I notice the functionality in the latest loop are all methods, not using properties:

for (var i = 0; i < ranges.length; i++) {
  if (ranges[i] != null && (!isnullorundefined(ranges[i].formulas[0][0]) && ranges[i].formulas[0][0] == '')) {
    ranges[i].clear(excel.clearapplyto.contents);
    if (clearformatting)
      ranges[i].clear(excel.clearapplyto.formats);
    ranges[i].untrack();
  }
 }

So the latest await context.sync(); seems could be removed, could you have a try on this?

jayakrishnankg commented 1 year ago

Hi @donlvMSFT , thanks for the reply.

  1. We're not sure about whether the issue was there from the beginning or introduced later on, Office version number: 16.75.2.
  2. It's a macro workbook with more than 25 worksheets and its size is around 1.5 MB.

As per your suggestions, we removed context.sync() , it saved execution time for that function only. The context.sync call at other functions/places still takes long time. We don't think removing context.sync will solve the problem because we need to sync the changes at least once in the execution cycle.

Please let us know if you need anything else, looking forward to your feedback.

donlvMSFT commented 1 year ago

Hi @jayakrishnankg ,

I could kindly repro, and we've created an internal item (8222682) to track this issue. We'll update here if there's any progress on the result.

Could you share the impact of this issue? That would help us to give correct priority for this work item.

jayakrishnankg commented 1 year ago

Hi @donlvMSFT,

Thanks for replying. We've several customers affected by this issue, and they are forced to close all the excel apps every time to run our add-in which is not good for our business. I'd insist you to keep it on high priority so that we don't lose our valuable customers citing performance issue. please provide a non-public channel to contact you guys. Is there an option for paid support assuming it would solve the issue quickly?

Looking forward to your feedback. Thanks.

jayakrishnankg commented 1 year ago

HI @donlvMSFT,

Do we have any updates on this issue? Please provide us with a way to track the progress on 8222682. We'd appreciate if we can at least get the response early.

Thanks.

donlvMSFT commented 1 year ago

Hi @jayakrishnankg ,

I'm not sure if you have channel with the MS support team, if so, you could reach out to them for help.

My e-mail is donlv@microsoft.com, you could drop e-mail to me. Also, could you share some details in the mail related to your add-in and also the usage?

Thanks,

jayakrishnankg commented 1 year ago

Hi @donlvMSFT,

I've sent you email, please check.

Looking forward to your response.

jayakrishnankg commented 1 year ago

Hi @donlvMSFT,

Any updates on this? It's turning to be quite critical now.

Thanks.