using ThreadLocalPtr to hide ROCKSDB_SUPPORT_THREAD_LOCAL from public… by lightmark · Pull Request #2380 · facebook/rocksdb

lightmark · 2017-05-27T02:30:25Z

… headers

#2199 should not reference RocksDB-specific macros (like ROCKSDB_SUPPORT_THREAD_LOCAL in this case) to public headers, iostats_context.h and perf_context.h. We shouldn't do that because users have to provide these compiler flags when building their binary with RocksDB.

We should hide the thread local global variable inside our implementation and just expose a function api to retrieve these variables. It may break some users for now but good for long term.

make check -j64

… headers

igorcanadi · 2017-05-29T20:58:25Z

Thanks @lightmark , this will fix the problem, although it might be a bit disruptive, since it will cause any code that uses perf_context to stop compiling. To help out with the conversion, I'd add #define ROCKSDB_PERF_CONTEXT_FUNCTION, so that people can do

#ifdef ROCKSDB_PERF_CONTEXT_FUNCTION
GetPerfContext() something
#else
perf_context something
#endif

in their codebases.

An alternative would be to create a build-process-generated rocksdb-defs.h public header file that would be full of necessary#defines.

siying

I didn't see a modification of HISTORY.md. It is very important to mention the breaking interface change there.

siying · 2017-05-30T14:45:23Z

include/rocksdb/perf_context.h

-#endif
+// Get Thread-local PerfContext object pointer
+// if defined(NPERF_CONTEXT), then the pointer is not thread-local
+PerfContext* GetPerfContext();


I think the naming convention is get_perf_context().

What do you think about @igorcanadi 's comment?

i think do it :), applications should be able to write code compatible across rocksdb minor versions

facebook-github-bot · 2017-05-31T02:04:16Z

@lightmark updated the pull request - view changes

facebook-github-bot · 2017-06-01T00:43:14Z

@lightmark updated the pull request - view changes

facebook-github-bot · 2017-06-01T00:56:32Z

@lightmark updated the pull request - view changes

lightmark · 2017-06-01T18:38:53Z

@igorcanadi What is the benefit of this MACRO over a version guard?

facebook-github-bot · 2017-06-01T23:01:31Z

@lightmark updated the pull request - view changes

igorcanadi · 2017-06-02T17:43:33Z

@igorcanadi What is the benefit of this MACRO over a version guard?

That works too. I actually think that the best solution would be to hack the build process to generate rocksdb-defines.h file. This is standard practice: https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

lightmark · 2017-06-02T18:14:23Z

@igorcanadi Let me read. I am not a cmake expert :(
I think even we should generate a rocksdb-defines.h file, it'd better to make a new PR.

facebook-github-bot · 2017-06-03T00:00:44Z

@lightmark has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: #2380 introduces a regression by replacing __thread with ThreadLocalPtr. Revert the thread local implementation back. Closes #2485 Differential Revision: D5308050 Pulled By: lightmark fbshipit-source-id: 2676e9c22edf76e8133d3f4c50e2711e11a95480

Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 update-submodule: rocksdb Reviewed By: alxyang Differential Revision: D5520030 fbshipit-source-id: 2b20883

Upstream commit ID : fb-mysql-5.6.35/2e0ecaf5ce8cb4a1bb1debbe2269839d88de6f1d Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 update-submodule: rocksdb Reviewed By: alxyang Differential Revision: D5520030 fbshipit-source-id: 2b20883

Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 @update-submodule: rocksdb Test Plan: sandcastle and make sure sysbench is back Reviewers: alexyang Reviewed By: alexyang Subscribers: webscalesql-eng@fb.com Differential Revision: https://phabricator.intern.facebook.com/D5520030 Signature: t1:5520030:1501275061:d8cca48eb4c82a7ed51e4a11f97947b996970ed5

Upstream commit ID : fb-mysql-5.6.35/2e0ecaf5ce8cb4a1bb1debbe2269839d88de6f1d Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 update-submodule: rocksdb Reviewed By: alxyang Differential Revision: D5520030 fbshipit-source-id: 2b20883

Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 update-submodule: rocksdb Reviewed By: alxyang Differential Revision: D5520030 fbshipit-source-id: 2d04833

Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 update-submodule: rocksdb Differential Revision: D5520030 (a27984c) fbshipit-source-id: 0da3e86fb08

Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 update-submodule: rocksdb Differential Revision: D5520030 (facebook@a27984c) fbshipit-source-id: 0da3e86fb08

Summary: The rocksdb PR facebook/rocksdb#2485 reverted an implementation of rocksdb perf and iostat counters to __thread from rocksdb::ThreadLocal. This change was previously done by PR facebook/rocksdb#2380 @update-submodule: rocksdb Differential Revision: D5520030

using ThreadLocalPtr to hide ROCKSDB_SUPPORT_THREAD_LOCAL from public…

8092459

… headers

facebook-github-bot added the CLA Signed label May 27, 2017

lightmark requested a review from siying May 27, 2017 02:30

lightmark mentioned this pull request May 27, 2017

remove #include port/port.h in public header file #2336

Closed

siying suggested changes May 30, 2017

View reviewed changes

change the name of functions and update history.md

ffc7634

add ROCKSDB_HIDE_THREAD_LOCAL_CONTEXT

11e7845

update HISTORY.md

58091e9

remove macro

79ffe40

siying approved these changes Jun 2, 2017

View reviewed changes

facebook-github-bot closed this in 7f6c02d Jun 3, 2017

lightmark mentioned this pull request Jun 5, 2017

bump version to 5.6 #2411

Closed

lightmark mentioned this pull request Jun 23, 2017

revert perf_context and io_stats to __thread #2485

Closed

Conversation

lightmark commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorcanadi commented May 29, 2017

Uh oh!

siying left a comment

Choose a reason for hiding this comment

Uh oh!

siying May 30, 2017

Choose a reason for hiding this comment

Uh oh!

lightmark May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajkr May 31, 2017

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 31, 2017

Uh oh!

facebook-github-bot commented Jun 1, 2017

Uh oh!

facebook-github-bot commented Jun 1, 2017

Uh oh!

lightmark commented Jun 1, 2017

Uh oh!

facebook-github-bot commented Jun 1, 2017

Uh oh!

igorcanadi commented Jun 2, 2017

Uh oh!

lightmark commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lightmark commented May 27, 2017 •

edited

Loading

lightmark May 30, 2017 •

edited

Loading

lightmark commented Jun 2, 2017 •

edited

Loading