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

Account for sparse vertex lists #432

Closed
wants to merge 1 commit into from

Conversation

jugglinmike
Copy link
Contributor

Because any vertex may be removed from the graph at any time, the array
that describes the vertex-to-index mapping may be sparse. Generate an
array with nil values in place of removed vertices.

Because any vertex may be removed from the graph at any time, the array
that describes the vertex-to-index mapping may be sparse. Generate an
array with nil values in place of removed vertices.
@jugglinmike
Copy link
Contributor Author

This bug causes redundant compilation, so a higher-level test may be appropriate.

@denisdefreyne
Copy link
Member

Hmm, I get the feeling there's an underlying issue here. I have access to your site source, so I’ll take a look.

@jugglinmike
Copy link
Contributor Author

Hey @ddfreyne. I think this is a bug in nanoc. Let me explain

The DirectedGraph's internal vertices hash is keyed on the size of the hash as each vertex is added. If any vertex but the "newest" is removed from this hash at any time, the keys will no longer be contiguous.

# example internal @vertices data structure
{
  1 => [:item, 'example1'],
  3 => [:item, 'example3']
}

The computed edges property is composed of tuples based on the (potentially non-contiguous) hash keys.

# example edges as computed by `DirectedGraph#edges`
[
  [1, 3],
  [3, 1]
]

So far so good. The problem comes when DependencyTracker stores the state of the graph. Here, it uses the computed vertices data structure. This data structure is a "defragmented" form of the internal @vertices hash.

# example vertices as computed by `DirectedGraph#vertices
[
  [:item, 'example1'],
  [:item, 'example3']
]

When DependencyTracker loads the data, it uses these two computed structures to re-build the graph. This inconsistency can cause nanoc to behave as if some items have been removed. This is the bug that I'm seeing on my site, where running rm -rf tmp && nanoc compile && nanoc compile triggers re-compilation of representations whose items have not changed.

@denisdefreyne
Copy link
Member

Oh wow, that is a nasty issue. Thanks for the detailed analysis!

Where are you removing items? Explicitly in the preprocess block?

@jugglinmike
Copy link
Contributor Author

I'm not explicitly removing any items. I guess I'll have to dig around in my site's layouts for suspicious inlined Ruby...

@jugglinmike
Copy link
Contributor Author

To make this process a little easier, I made the following change:

diff --git a/Rules b/Rules
index c522a62..fc55774 100755
--- a/Rules
+++ b/Rules
 # preprocess lets you access the @items array before any compilation
 preprocess do
+  @items.freeze

This immediately failed because I am currently (intentionally) creating new items to model "categories" on my site.

I commented out the lines that push new "category" items into the @items array, and the site compiled successfully, which makes me believe there is no further modification to the items array. The site also continued to exhibit the same overcompilation behavior, so the "category" creation is not causing this issue.

Is my approach to detecting item removal sound? With this change, is there any other way that my code could be removing items?

@denisdefreyne
Copy link
Member

Yep, @items.freeze will prevent modifications at all.

Can you try using the release-3.6.x branch and seeing whether that branch still exhibits the problem? The upcoming nanoc patch version includes improvements to the checksummer, which determines whether or not objects have changed, so that improvement can be relevant here.

@denisdefreyne
Copy link
Member

I’m getting the feeling that we’re on the wrong track with the item removal idea… nanoc calculates outdated items recursively, so a single compilation should find everything.

I’ll play around with the site source (I have access to it) later today and see what I can find out!

@jugglinmike
Copy link
Contributor Author

@ddfreyne correctly identified the underlying problem as a user error (and here I thought I was so clever with my sparse arrays). Further, he has landed #435 to provide an explicit error in the case that others make the same mistake. Thank a lot, Denis!

@denisdefreyne
Copy link
Member

Sweet, good to know this got resolved!

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.

None yet

2 participants