Skip to content

update abseil#7146

Closed
kyessenov wants to merge 7 commits into
envoyproxy:masterfrom
kyessenov:abseil
Closed

update abseil#7146
kyessenov wants to merge 7 commits into
envoyproxy:masterfrom
kyessenov:abseil

Conversation

@kyessenov

Copy link
Copy Markdown
Contributor

Signed-off-by: Kuat Yessenov kuat@google.com

Description: Update abseil library to 5/17
Risk Level: Low
Testing: No extra
Docs Changes: None
Release Notes:

kyessenov added 3 commits June 3, 2019 15:00
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov

Copy link
Copy Markdown
Contributor Author

cc @g-easy : trying to divide and conquer your PR.

@g-easy

g-easy commented Jun 3, 2019

Copy link
Copy Markdown
Contributor

Thanks! :)

@kyessenov

Copy link
Copy Markdown
Contributor Author

This comes with a fix to use node_hash_map instead of flat_hash_map. The difference is that the former does not move the values on the table re-organization. This prevents the double destructor call which appears to cause the ASAN violation. I think the upgrade of absl changed the heuristics in the hash map, so it re-orged in the unit test. That doesn't mean the error is absent on master, it just revealed itself after the upgrade.

/cc @lizan

Signed-off-by: Kuat Yessenov <kuat@google.com>
Comment thread source/server/server.h Outdated
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov

Copy link
Copy Markdown
Contributor Author

I'm not sure how to deal with ipv6 test. I'm getting it to pass locally:

bazel test --test_env=ENVOY_IP_TEST_VERSIONS=v6only //test/integration:stats_integration_test

@lizan

lizan commented Jun 4, 2019

Copy link
Copy Markdown
Member

@kyessenov what's the version of libstdc++ in your local? The m_per_cluster is different across environments, so just set it what CI says, not your local one.

@lizan

lizan commented Jun 4, 2019

Copy link
Copy Markdown
Member

This comes with a fix to use node_hash_map instead of flat_hash_map. The difference is that the former does not move the values on the table re-organization. This prevents the double destructor call which appears to cause the ASAN violation. I think the upgrade of absl changed the heuristics in the hash map, so it re-orged in the unit test. That doesn't mean the error is absent on master, it just revealed itself after the upgrade.

Nice catch, it is worth to submit that part as separate PR, with a regression test.

@kyessenov

Copy link
Copy Markdown
Contributor Author

@lizan

How do I satisfy both versions of libstdc++? I see that the regular test run matches my local environment and expects 49193 in the test. But then IPv6 run must be using a different environment and expects a different number 49137. Do you want me to put both numbers in the test?

Signed-off-by: Kuat Yessenov <kuat@google.com>
@lizan

lizan commented Jun 4, 2019

Copy link
Copy Markdown
Member

@kyessenov I think you should only put 49137, which should pass all CI. That test won't pass in local environments.

Signed-off-by: Kuat Yessenov <kuat@google.com>
@lizan lizan self-assigned this Jun 4, 2019
@kyessenov

Copy link
Copy Markdown
Contributor Author

Is there anything I can do to pass macos build? I don't have a mac so not sure how to debug the CI failure.

@lizan

lizan commented Jun 4, 2019

Copy link
Copy Markdown
Member

It seems failing with thread annotation, @jmarantz do you have any idea from abseil changes?

@jmarantz jmarantz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not know why this would start failing now. The thread annotation is a function of clang more than absl.

// 2019/06/03 7146 49137 absl update

EXPECT_EQ(m_per_cluster, 50157);
EXPECT_EQ(m_per_cluster, 49137);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice reduction of almost 1k per cluster. I'm surprised absl made that much of a difference. I assume this is not because o of the service node_hash_map, which I don't think would be replicated per cluster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reduction was before I made node_hash_map change I think.

@kyessenov

Copy link
Copy Markdown
Contributor Author

Is there anything I can do to debug macos build? I don't have a mac laptop. Should I borrow it from someone to reproduce locally?

@lizan

lizan commented Jun 6, 2019

Copy link
Copy Markdown
Member

@kyessenov I'm working on a workaround, do you mind me take over this PR? Upgrading absl to more recent version and use absl thread annotation would solve it.

@kyessenov

Copy link
Copy Markdown
Contributor Author

Sure, go ahead.

@lizan lizan mentioned this pull request Jun 7, 2019
htuch pushed a commit that referenced this pull request Jun 7, 2019
Update absl to latest and enable in macOS. Retrying #7146.

Risk Level: Low

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@kyessenov

Copy link
Copy Markdown
Contributor Author

Superseded by #7199

@kyessenov kyessenov closed this Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants