Remove all time inefficient loops from mlbf

[KevinMind]

Fixes: mozilla/addons#15170

Description

• extract the database querying logic from MLBFDatabaseLoader to module scope functions for easier mocking
• reduce number of loops required to build initial dataset in MLBFDatabaseLoader to 1
• filter not_blocked_items with an O(1) set instead of list
• refactor generate_diffs to accept a block_type and only diff one set of data at a time.
• add an lru_cache to generate_diffs to ensure computation is executed only one time per instance + block_type + previous_mlbf
• collect all mlbf tests under test_mlbf mark

Context

This patch includes logic that should reduce the number of long loops required to process our mlbf data sets and determine the cache and diff of blocks across different scenarios.

Testing

Run the mlbf suite

make test_mlbf

This will run all of the mlbf related tests. Expect everything passes

Test the timeout for the slow test

1. modify the pytest.mark.timeout(120) reducing the time to 3 (or some low number
2. run the test

pytest ./src/olympia/blocklist/tests/test_cron.py::TestUploadToRemoteSettings::test_large_data_set

3. expect the test to fail due to timeout

FAILED src/olympia/blocklist/tests/test_cron.py::TestUploadToRemoteSettings::test_large_data_set - Failed: Timeout >10.0s

Test in local environment

IF you want to test with a real data set you will have to create a large amount of blocks and execute the cron

Code snippet

Execute this code. You will need to execute many time or bump `1_000 up to between 1 and 2 million.. grab a coffee this will take a while.

After that is finished, execute ./manage.py cron upload_mlbf_to_remote_settings and expect it to take up to 2 minutes to complete.

Checklist

• Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
• Successfully verified the change locally.
• The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
• Add before and after screenshots (Only for changes that impact the UI).
• Add or update relevant docs reflecting the changes made.

[willdurand]

Thanks for adding the new tests, that looks good at first glance. I'm going to focus on #22828 so I'll let someone else take care of this PR.

Fixes: mozilla/addons#15170

I'd suggest filing a new issue for this, it could be about adding tests (and fixing more things revealed by the tests)

[eviljeff]

wdyt?

[eviljeff]

Can we just use sets instead? They're much more efficient for comparison, and, to me, sticking to sets everywhere would be more straightforward.

We only need a specific order when we're writing to JSON, (because it doesn't support sets), so we could just sort the data just before writing.

[KevinMind]

I'm afraid chat GPT and myself disagree with you on this one. Here's the breakdown.

TLDR; we save no time (lose a little bit actually) using set difference. On top of that it is less space efficient, and as you said does not preserve order. I see no reason to prefer it based on merit.

In the current implementation we have:

• Building previous_set: This takes O(len(previous)) time.
• List comprehension:
  • Iterates over current, which is O(len(current)).
  • The x not in previous_set check is O(1) per element due to the constant-time lookup in a set.

Total Time Complexity: O(len(current) + len(previous))
Space Complexity: O(len(previous)) for previous_set.

In the set only option:

• Building set(current): O(len(current))
• Building set(previous): O(len(previous))
• Set difference set(current) - set(previous):
  • Iterates over set(current), which is O(len(set(current))).
  • For each element, checks if it's in set(previous), which is O(1) per element.
• Converting the set difference to a list: O(len(result))

Total Time Complexity: O(2 X len(current) + len(previous))
Space Complexity: O(len(current) + len(previous)) for both sets.

Comparison:

• Time Complexity: Both functions have the same time complexity of O(len(current) + len(previous)).
• Space Complexity: Function 1 uses less additional space because it only builds one set (previous_set), whereas Function 2 builds two sets (set(current) and set(previous)).
• Order Preservation: Function 1 preserves the order of elements in current, while Function 2 does not, because sets are unordered collections.

[eviljeff]

If we're just considering this function chatGPT may have a point, but I meant use sets throughout the code in this file; then we can use more efficient set arithmetic and drop this function. (And do sorted(...) only when we're writing json)

[KevinMind]

FWIW I wasn't considering just this function and it doesn't make a difference in any case. Using set arithmetic is going to result in more space usage and the same amount of time usage. It simply does not benefit us in this use outside of perhaps looking nicer.

Additionally, one of the issues we had with the approach of sorting before writing is the built in sort method didn't seem to work predictably causing really flaky tests.. I think we'd have to figure that out if we wanted to switch to sets.

[eviljeff]

Suggested change

	for guid, version, id, block_type in get_all_blocked_items():
	_blocked_version_ids.append(id)
	_blocked[block_type].append((guid, version))
	for guid, version_string, version_id, block_type in get_all_blocked_items():
	_blocked_version_ids.append(version_id)
	_blocked[block_type].append((guid, version_string))

[eviljeff]

Though I do wonder if it's more efficient to generate _blocked_version_ids and _blocked with list/set generator syntax, rather than loop through with a for loop and append one at a time.

[KevinMind]

Well this loop will be an O(n) over all blocked items.
And there will be 2 O(1) appends per iteration.. so still O(n)

Do you have an example of something that would be more efficient? I thnk this doesn't look as pretty but is actually the efficient way to go.

[eviljeff]

I was thinking more that, in python anyway, it's generally more efficient to build a list with a generator than to build it by iterating and repeatedly appending items (because it needs resizing and reallocating more). I don't have stats to back-up the difference between generating two lists vs. iterating over the list once though.

[KevinMind]

Yeah IDK about the perf difference there, but I imagine it would not be significant since we are comparing a slightly slower single iteration versus a slightly faster double iteration. We could give it a try and see if there is a big diff.

[eviljeff]

... yeah? I remain in the pro-set camp though. And I'd like someone who's been more involved with the intricacies of the current soft-blocking work (e..g @willdurand) to give this a once-over too.

(I understand this may not be merged for a while anyway, if we're soft-freezing blocklisting changes for QA)

[eviljeff]

If we're just considering this function chatGPT may have a point, but I meant use sets throughout the code in this file; then we can use more efficient set arithmetic and drop this function. (And do sorted(...) only when we're writing json)

[eviljeff]

I was thinking more that, in python anyway, it's generally more efficient to build a list with a generator than to build it by iterating and repeatedly appending items (because it needs resizing and reallocating more). I don't have stats to back-up the difference between generating two lists vs. iterating over the list once though.

[eviljeff]

nit: I don't think we're ever going to call the function 128 times 🙃 - it's typically like, 3 times, isn't it?

[KevinMind]

I think it can be more than three but yeah probably not 128. I think I copy pasted that from docs.. maybe there is a sensible default.

[eviljeff]

120 or 180?

[diox]

Just to make it clear: because QA is testing things right now and this is the last push of the year, we'd like to avoid big changes to blocklisting, so this should NOT land in this sprint, even if approved.