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

Reduce dependency bloat #1113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Reduce dependency bloat #1113

wants to merge 3 commits into from

Conversation

Kixiron
Copy link
Contributor

@Kixiron Kixiron commented Nov 12, 2021

  • Started trying to remove num from the dependency tree
  • Removed some of the "default dependencies" from generated crates, this should allow better compilation scheduling since crates don't have to wait on unnecessary dependencies
  • Discovered that some files are in CLRF form, this will be fixed in a later PR
  • Removed dependencies on fnv and twox-hash and replaced with a single dependence on xxhash-rust, it has zero dependencies and is very light
  • Removed old dependence on time for all generated crates, this reduced the dependency tree a good bit
  • Minor refactors to some massive match statements
  • Added some documentation to the DDVal/DDValue innards

@vmwclabot

This comment has been minimized.

@mihaibudiu
Copy link

mihaibudiu commented Nov 12, 2021

Some of these files show every line as different. Is this due to CR/LF?
This makes it hard to review.

@Kixiron
Copy link
Contributor Author

Kixiron commented Nov 12, 2021

Yes, those are files that were changed from CLRF to LF, I'll make a PR that bites the bullet and standardizes things after this.
In the diff view select the Hide Whitespace option to ignore the line ending changes

image

Copy link
Contributor

@ryzhyk ryzhyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of test failures. Some of them are probably caused by full disks on CI machines. Those should now be fixed. Others probably just require changing reference test outputs due to the use of a different hashing function.

I think most of the maps that use FNV today can be regular default HashMaps. I mostly used FNV to ensure deterministic iteration order. This is necessary e.g., when building dataflow graphs to make sure that each DD worker ends up with the same operator id assignment. In most other cases there's no reason for non-standard hasher.

rust/template/differential_datalog/src/ddval/mod.rs Outdated Show resolved Hide resolved

const XX_SEED1: u64 = 0x23b691a751d0e108;
const XX_SEED2: u64 = 0x20b09801dce5ff84;

#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we trust the compiler with all this inlining?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, however this makes the functions viable for cross-crate inlining

@vmwclabot

This comment has been minimized.

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.

4 participants