MWPW-159113 [Personalization] Parallelize personalization network requests to be as efficient as possible #2970

sharmrj · 2024-09-27T06:57:40Z

Network requests, entitlements.js, promo-utils.js (if needed), and the manifest files are now made concurrently.

This optimization won't immediately bear fruit since it reveals a different bottleneck
Before:

After:

Once entitlements.js, promo-utils.js, and the manifests load concurrently, we find that launch becomes our bottleneck, so thi optimization needs to be combined with two subsequent optimizations in order to bear fruit. Note that photoshop.json takes less time to load in prod environments, so launch is more likely to be bottleneck in prod.

Launch.js needs to be split up/optimized. Related tickets: https://jira.corp.adobe.com/browse/MWPW-158739, https://jira.corp.adobe.com/browse/MWPW-158749
The request for the manifest files may need to be made earlier (this depends on how long it takes to load in prod with the prior optimizations, since manifest files take less time to laod on prod.). We'll have to wait for prior optimizations to be done to decide the next step.

Resolves: MWPW-159113

Test URLs:

https://main--cc--adobecom.hlx.page/products/photoshop?milolibs=par-pers--milo--sharmrj

… and the manifest

aem-code-sync · 2024-09-27T06:57:58Z

	Page	Scores	Audits	Google
📱	/?martech=off
🖥️	/?martech=off

codecov · 2024-09-27T06:58:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.80%. Comparing base (f950ca5) to head (3180394).

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #2970      +/-   ##
==========================================
+ Coverage   95.92%   98.80%   +2.87%     
==========================================
  Files         175       70     -105     
  Lines       47971     8449   -39522     
==========================================
- Hits        46016     8348   -37668     
+ Misses       1955      101    -1854

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mokimo

Very cool to see this being tackled, with MEP/Personalization we should have room for improvements in terms of performance.

mokimo · 2024-09-27T09:22:50Z

libs/features/personalization/personalization.js

  }

+  manifests = await Promise.all((await Promise.all(manifestPromises))


Is the double Promise.all there for any specific reason, or is that an omission?

// Untested demo code manifests = await Promise.all( manifestPromises.map(async (promise) => { try { const resp = await promise; if (!resp.ok) { if (resp.status === 404) { throw new Error('File not found'); } throw new Error(`Invalid response: ${resp.status} ${resp.statusText}`); } const manifestData = await resp.json(); return { manifestData, manifestPath: resp.url }; } catch (e) { console.log(`Error loading content: ${resp.url}`, e.message || e); return null; } }) ).then(results => results.filter(Boolean));

I agree that the double Promise.all needs to be refactored. I've done something similar to what you've suggested.

sharmrj · 2024-09-27T11:11:45Z

@mokimo pointed out an issue where we're waiting a long time in between the interact call and loading the blocks.

Turns out, if launch finishes loading before the ims token network request completes, the page stalls for 4 extra seconds while we wait for the alloy event (even though the interact call goes through just fine). I've confirmed this by making measurements of getTargetPersonalization like so:

    performance.mark('targetstart');
    const { targetManifests, targetPropositions } = await getTargetPersonalization();
    performance.mark('targetend');
    console.log(performance.measure('target', 'targetstart', 'targetend'));

If launch loads before the ims token comes back we always get a 4second timing on this function:

PerformanceMeasure {detail: null, name: 'target', entryType: 'measure', startTime: 5821.900000095367, duration: 4000.899999976158}

Note that the timeout duration in martech.js is exactly 4000ms.
If the ims token comes back before launch we get something more along the lines of:

PerformanceMeasure {detail: null, name: 'target', entryType: 'measure', startTime: 8346.5, duration: 596.2999999523163}

This is possibly why some Nala tests are timing out.

AFAICT this sometimes happens out in the wild, it's just that the optimization in this pr makes it more likely to happen. I'll continue to investigate the root cause.

github-actions · 2024-09-28T01:35:11Z

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

github-actions · 2024-10-06T01:39:17Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

vgoodric

Please let me know when this is finalized so we can thoroughly QA it. SOT approval often does not check for MEP.

github-actions · 2024-10-16T01:35:17Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

3ch023 · 2024-10-17T12:00:18Z

libs/features/personalization/personalization.js

  }

  if (promoEnabled) {
-    const { default: getPromoManifests } = await import('./promo-utils.js');
+    const { default: getPromoManifests } = await promoUtilsPromise;


we could also move code from promo-utils.js directly here, i think it was not a good decision in the first place to have a separate file.
promo-utils.js contents seem small enough

3ch023 · 2024-10-17T12:07:08Z

libs/features/personalization/personalization.js

@@ -1037,15 +1047,22 @@ export const combineMepSources = async (persEnabled, promoEnabled, mepParam) =>
      }
    });
  }
-  return persManifests;
+  return initialPersManifestsPromises.concat(persManifests
+    .filter((m) => !('disabled' in m) || !m.disabled)


if i remember correct, we shouldn't filter out disabled manifests.
those are 'promotions' manifests that are schedule is in the future/past, but Content Authors need to see them in the Page mep preview interface. However we don't load the .json itself (at least this is how it used to work) unless the manifest is active.

if you are still able to see a disabled manifest in the page then we are good

github-actions · 2024-10-25T01:27:13Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

github-actions · 2024-11-02T01:25:34Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

sharmrj · 2024-11-07T07:58:26Z

For whatever reason I'm seeing the timeout issue again after merging the stage branch into my branch and fixing the merge conflicts. I was also unable to reproduce the issue earlier which is why I thought it had been resolved.
It seems IMS loaded after launch and that's what triggered the issue again:

I'll attach the trace here:
Trace-20241107T131821.json.zip

chrischrischris · 2024-11-08T13:20:02Z

libs/features/personalization/personalization.js

+      const normalizedURL = normalizePath(manifestPath);
+      return fetch(normalizedURL, { mode: 'same-origin' });


nit: inline normalizePath

chrischrischris · 2024-11-08T13:27:42Z

libs/features/personalization/personalization.js

+        /* c8 ignore next 3 */
+        console.log(`Error loading content: ${resp.url}`, e.message || e);
+      }
+      if (pzn) loadLink(getXLGListURL(config), { as: 'fetch', crossorigin: 'anonymous', rel: 'preload' });


Doesn't this line still need to run even when the manifest loading is successful?

sharmrj added 7 commits September 25, 2024 15:13

Parallelized the network requests to promo-utils.js, entitlements.js,…

b28d725

… and the manifest

Merge branch 'stage' of https://github.com/adobecom/milo into par-pers

40949a2

added manifestPath to fetched manifests

afd675d

placeholder

6324d09

revert previous change

ba72b1c

Merge branch 'stage' of https://github.com/adobecom/milo into par-pers

9eacd32

Unit tests

ed95461

sharmrj requested a review from a team as a code owner September 27, 2024 06:57

spadmasa self-assigned this Sep 27, 2024

sharmrj added 5 commits September 27, 2024 13:56

Fixed actionsTargetManifestId tests

d33f201

Fixed lint errors

49609b3

More lint fixes

c9f170d

utils-mep tests

fd4a085

utils.test.js

bbaf789

mokimo reviewed Sep 27, 2024

View reviewed changes

removed double await Promise.all

ae83c48

github-actions bot added the Stale label Oct 6, 2024

mokimo requested review from vgoodric and AdobeLinhart October 8, 2024 17:03

vgoodric requested changes Oct 8, 2024

View reviewed changes

github-actions bot removed the Stale label Oct 9, 2024

github-actions bot added Stale and removed Stale labels Oct 16, 2024

3ch023 reviewed Oct 17, 2024

View reviewed changes

github-actions bot added Stale and removed Stale labels Oct 25, 2024

github-actions bot added the Stale label Nov 2, 2024

Merge branch 'stage' of https://github.com/adobecom/milo into par-pers

c388e34

sharmrj removed the Stale label Nov 7, 2024

removed the import of entitlements.js

3180394

chrischrischris reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWPW-159113 [Personalization] Parallelize personalization network requests to be as efficient as possible #2970

MWPW-159113 [Personalization] Parallelize personalization network requests to be as efficient as possible #2970

sharmrj commented Sep 27, 2024 •

edited

Loading

aem-code-sync bot commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

mokimo left a comment

mokimo Sep 27, 2024

sharmrj Sep 27, 2024

sharmrj commented Sep 27, 2024 •

edited

Loading

github-actions bot commented Sep 28, 2024

github-actions bot commented Oct 6, 2024

vgoodric left a comment

github-actions bot commented Oct 16, 2024

3ch023 Oct 17, 2024

3ch023 Oct 17, 2024

github-actions bot commented Oct 25, 2024

github-actions bot commented Nov 2, 2024

sharmrj commented Nov 7, 2024 •

edited

Loading

chrischrischris Nov 8, 2024

chrischrischris Nov 8, 2024

		}

		manifests = await Promise.all((await Promise.all(manifestPromises))

		const normalizedURL = normalizePath(manifestPath);
		return fetch(normalizedURL, { mode: 'same-origin' });

MWPW-159113 [Personalization] Parallelize personalization network requests to be as efficient as possible #2970

Are you sure you want to change the base?

MWPW-159113 [Personalization] Parallelize personalization network requests to be as efficient as possible #2970

Conversation

sharmrj commented Sep 27, 2024 • edited Loading

aem-code-sync bot commented Sep 27, 2024 • edited Loading

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

mokimo left a comment

Choose a reason for hiding this comment

mokimo Sep 27, 2024

Choose a reason for hiding this comment

sharmrj Sep 27, 2024

Choose a reason for hiding this comment

sharmrj commented Sep 27, 2024 • edited Loading

github-actions bot commented Sep 28, 2024

github-actions bot commented Oct 6, 2024

vgoodric left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024

3ch023 Oct 17, 2024

Choose a reason for hiding this comment

3ch023 Oct 17, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2024

github-actions bot commented Nov 2, 2024

sharmrj commented Nov 7, 2024 • edited Loading

chrischrischris Nov 8, 2024

Choose a reason for hiding this comment

chrischrischris Nov 8, 2024

Choose a reason for hiding this comment

sharmrj commented Sep 27, 2024 •

edited

Loading

aem-code-sync bot commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

sharmrj commented Sep 27, 2024 •

edited

Loading

sharmrj commented Nov 7, 2024 •

edited

Loading