Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349163003:
experiment/fiddle (Closed)
|
Created:
5 years, 3 months ago by hal.canary Modified:
5 years ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionexperiment/fiddle
Committed: https://skia.googlesource.com/skia/+/decb21e3ae3d296976d8664e49e35971d1b4fadd
Patch Set 1 #
Total comments: 21
Patch Set 2 : 2015-11-19 (Thursday) 12:50:41 EST #Patch Set 3 : working again #Patch Set 4 : 2015-11-19 (Thursday) 16:38:20 EST #Patch Set 5 : 2015-11-19 (Thursday) 17:46:44 EST #Patch Set 6 : 2015-11-19 (Thursday) 18:17:17 EST #Patch Set 7 : 2015-11-19 (Thursday) 18:20:02 EST #Patch Set 8 : 2015-11-24 (Tuesday) 12:12:16 EST #
Total comments: 1
Patch Set 9 : 2015-12-01 (Tuesday) 15:01:54 EST #Patch Set 10 : 2015-12-01 (Tuesday) 15:05:27 EST #
Total comments: 4
Patch Set 11 : changes for jcgregorio@ #
Total comments: 1
Patch Set 12 : 2015-12-10 (Thursday) 10:34:44 EST #
Messages
Total messages: 39 (14 generated)
halcanary@gmail.com changed reviewers: + halcanary@gmail.com, mtklein@google.com
PTAL at changes to cmake. experimental/fiddle is a potential replacement for skfiddle that should compile faster and only touch the public API.
halcanary@google.com changed reviewers: - halcanary@gmail.com
mtklein@google.com changed reviewers: + bsalomon@google.com
mtklein@google.com changed required reviewers: + bsalomon@google.com
https://codereview.chromium.org/1349163003/diff/1/cmake/CMakeLists.txt File cmake/CMakeLists.txt (right): https://codereview.chromium.org/1349163003/diff/1/cmake/CMakeLists.txt#newcode26 cmake/CMakeLists.txt:26: find_include_dirs(private_includes ../src/*.h ../include/private/*.h) add include/config/*.h here instead? https://codereview.chromium.org/1349163003/diff/1/cmake/CMakeLists.txt#newcod... cmake/CMakeLists.txt:206: if(${CMAKE_BUILD_TYPE} STREQUAL "Debug") This seems unnecessary. SK_DEBUG / SK_RELEASE are set by the presence of NDEBUG. https://codereview.chromium.org/1349163003/diff/1/cmake/CMakeLists.txt#newcod... cmake/CMakeLists.txt:213: configure_file(SkUserConfig.h.in SkUserConfig.h) Let's not do this. I haven't seen any need yet for a non-default (non-empty) SkUserConfig.h. https://codereview.chromium.org/1349163003/diff/1/cmake/CMakeLists.txt#newcod... cmake/CMakeLists.txt:220: if (UNIX AND NOT APPLE AND SK_MESA) Why is this opt-in? Can we not just detect it like the other dependencies? https://codereview.chromium.org/1349163003/diff/1/cmake/CMakeLists.txt#newcod... cmake/CMakeLists.txt:249: install(DIRECTORY ../include/core DESTINATION include/skia) Yeah, again, I think it's a bad idea to even insinuate we should be installing headers and skia places. Particularly true with the C++ API. If you want to do this for fiddle, do it in a script outside here? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefile File experimental/fiddle/Makefile (right): https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:14: CXXFLAGS += -Wall -Werror --std=c++11 Why do we -Wall -Werror? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:26: ninja -C build install I'm skeptical of all these different directories and installation. Why can't you just build in ../../cmake and use headers from ../../include? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:44: skia.gch: skia.h Does this cut build times down significantly? I am skeptical of the added complexity. I would rather fiddle use Skia's headers as we and most users use them. https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:51: $(CXX) $(CXXFLAGS) $(SKIA_INCLUDES) -include skia.h -c -o $@ $< Don't we need to name it skia.h.gch for this to work? As written, are you not just using skia.h directly, making skia.gch moot? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:55: FIDDLE_LD := -lOSMesa -Wl,-rpath -Wl,"$(PREFIX)/lib" Shouldn't Skia be linked against OSMesa instead? Or does fiddle need to call OSMesa things itself too? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:57: FIDDLE_SOURCES := draw.cpp fiddle_main.o OSMesaContextHolder.o $(PREFIX)/lib/$(LIBSKIA) Why are fiddle_main and OSMesaContextHolder different steps? Why not just list the .cpp files here? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/Makefil... experimental/fiddle/Makefile:60: $(CXX) $(CXXFLAGS) $(SKIA_INCLUDES) -include skia.h -o $@ $(FIDDLE_SOURCES) $(FIDDLE_LD) Ditto about skia.h.gch. https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/OSMesaC... File experimental/fiddle/OSMesaContextHolder.h (right): https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/OSMesaC... experimental/fiddle/OSMesaContextHolder.h:12: class OSMesaContextHolder { Why is this code not just in fiddle_main? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/base64.h File experimental/fiddle/base64.h (right): https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/base64.... experimental/fiddle/base64.h:12: inline void EncodeToBase64(const void* data, size_t size, FILE* out) { Ditto. Why is this not in fiddle_main? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/fiddle_... File experimental/fiddle/fiddle_main.cpp (right): https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/fiddle_... experimental/fiddle/fiddle_main.cpp:16: SkImage* image(nullptr); Just curious: you seem to be managing this manually. Why not just make this an SkAutoTUnref<SkImage>? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/fiddle_... experimental/fiddle/fiddle_main.cpp:18: const char* FLAGS_source(nullptr); Any reason these FLAGS_ guys aren't static? https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/fiddle_... experimental/fiddle/fiddle_main.cpp:31: " --noraster Disable the raster backend.\n" Why are any of these options? Let's just run them all and, if you really want, not show the results on the web page? Again, not sure why we need that to be an option either. https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/fiddle_... experimental/fiddle/fiddle_main.cpp:34: fputs(options, stderr); If you use fprintf you can keep your (default is %d) correct. https://codereview.chromium.org/1349163003/diff/1/experimental/fiddle/fiddle_... experimental/fiddle/fiddle_main.cpp:93: perror("Unable to open the source image file."); I don't think you mean to be using perror. perror does print the message to stderr, yes, but also prints the error described by errno. Our error did not write to errno, so errno's going to be at the very least stale, probably confusing. You're going to get a lot of ": Undefined error: 0". Perhaps you meant: #define error(fmt, ...) fprintf(stderr, fmt"\n", ##__VA_ARGS__) https://codereview.chromium.org/1349163003/diff/1/include/gpu/gl/GrGLInterface.h File include/gpu/gl/GrGLInterface.h (right): https://codereview.chromium.org/1349163003/diff/1/include/gpu/gl/GrGLInterfac... include/gpu/gl/GrGLInterface.h:44: SK_API const GrGLInterface* GrGLCreateNativeInterface(); Uh, either these all should be SK_API or none should. Brian can say for sure, but I think you're doing something wrong to have to call these.
On 2015/09/17 13:12:33, halcanary_use_other_email wrote: > PTAL at changes to cmake. > > experimental/fiddle is a potential replacement for skfiddle that should compile > faster and only touch the public API. How much faster?
On 2015/09/17 13:58:50, mtklein wrote: > How much faster? Good question. I'm doing the last step in ~100ms. I need to do an apples-to-apples comparison against the static linking that fiddle does now.
On 2015/09/17 13:58:23, mtklein wrote: > [...] I'd rather just discuss these with you in person when you have a chance.
https://codereview.chromium.org/1349163003/diff/1/include/gpu/gl/GrGLInterface.h File include/gpu/gl/GrGLInterface.h (right): https://codereview.chromium.org/1349163003/diff/1/include/gpu/gl/GrGLInterfac... include/gpu/gl/GrGLInterface.h:44: SK_API const GrGLInterface* GrGLCreateNativeInterface(); On 2015/09/17 13:58:22, mtklein wrote: > Uh, either these all should be SK_API or none should. Brian can say for sure, > but I think you're doing something wrong to have to call these. I'm pretty sure they should all be SK_API, since you can't create a mesa GrContext without a GrGLInterface. The consensus in the office is that I should do make this change and ask Brian for forgiveness after he gets back.
On 2015/09/17 14:19:25, Hal Canary wrote: > https://codereview.chromium.org/1349163003/diff/1/include/gpu/gl/GrGLInterface.h > File include/gpu/gl/GrGLInterface.h (right): > > https://codereview.chromium.org/1349163003/diff/1/include/gpu/gl/GrGLInterfac... > include/gpu/gl/GrGLInterface.h:44: SK_API const GrGLInterface* > GrGLCreateNativeInterface(); > On 2015/09/17 13:58:22, mtklein wrote: > > Uh, either these all should be SK_API or none should. Brian can say for sure, > > but I think you're doing something wrong to have to call these. > > I'm pretty sure they should all be SK_API, since you can't create a mesa > GrContext without a GrGLInterface. The consensus in the office is that I should > do make this change and ask Brian for forgiveness after he gets back. This is unimportant enough and SK_API is important enough that I'd rather block your progress here than get this wrong.
On 2015/09/17 14:11:33, Hal Canary wrote: > On 2015/09/17 13:58:50, mtklein wrote: > > How much faster? > > Good question. I'm doing the last step in ~100ms. I need to do an > apples-to-apples comparison against the static linking that fiddle does now. Can you compare what you have here to what you have here stripped of anything mentioning skia.gch?
On 2015/09/17 14:21:27, mtklein wrote: > On 2015/09/17 14:11:33, Hal Canary wrote: > > On 2015/09/17 13:58:50, mtklein wrote: > > > How much faster? > > > > Good question. I'm doing the last step in ~100ms. I need to do an > > apples-to-apples comparison against the static linking that fiddle does now. > > Can you compare what you have here to what you have here stripped of anything > mentioning skia.gch? With precompiled headers: 0.117s Without precompiled headers: 0.505s
Description was changed from ========== Cmake: c++ api includes, SK_MESA option; experiment/fiddle ========== to ========== experiment/fiddle ==========
halcanary@google.com changed reviewers: + jcgregorio@google.com - bsalomon@google.com
halcanary@google.com changed required reviewers: - bsalomon@google.com
Joe, I'm adding you to this CL reference. Last I remember, I was working on figuring out which syscalls I needed on and which I could turn block. When I have a free day, I'll work on this again.
Please take a look. Everything here is done except for the secwrap stuff, which I can work on later. I suggest we go ahead and commit this now. Maybe we should have a bot which makes sure that this code continues to compile (like our cmake bots).
Patchset #11 (id:200001) has been deleted
Patchset #10 (id:180001) has been deleted
Patchset #9 (id:160001) has been deleted
Patchset #8 (id:140001) has been deleted
I have now made this CL simpler and used only Go to control
the fiddle build.
Example use:
cd ..../skia
SKIA=$PWD
cd experimental/fiddle
go build fiddler.go
# compile prerequisites
./fiddler "$SKIA"
# compile and run a fiddle
./fiddler "$SKIA" - < draw.cpp | ./parse-fiddle-output
# compile and run a different fiddle
./fiddler "$SKIA" - < ANOTHER_FIDDLE.cpp | ./parse-fiddle-output
The functions inside fiddler.go could be incorporated into a
larger webserver program.
https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... File experimental/fiddle/fiddler.go (right): https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... experimental/fiddle/fiddler.go:141: out, err := os.Create(path.Join(fiddle_dir, "skia.h")) Generating skia.h should be part of cmake, that way options used in building the library can be baked into the skia.h header itself.
On 2015/11/24 at 20:21:35, jcgregorio wrote: > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > File experimental/fiddle/fiddler.go (right): > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > experimental/fiddle/fiddler.go:141: out, err := os.Create(path.Join(fiddle_dir, "skia.h")) > Generating skia.h should be part of cmake, that way options used in building the library can be baked into the skia.h header itself. You'll never convince mtklein@ of that.
On 2015/11/24 at 20:45:28, halcanary wrote: > On 2015/11/24 at 20:21:35, jcgregorio wrote: > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > > File experimental/fiddle/fiddler.go (right): > > > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > > experimental/fiddle/fiddler.go:141: out, err := os.Create(path.Join(fiddle_dir, "skia.h")) > > Generating skia.h should be part of cmake, that way options used in building the library can be baked into the skia.h header itself. > > You'll never convince mtklein@ of that. If you want to create a header file for user definitions, please use the existing SkUserConfig.h.
On 2015/11/25 at 16:01:29, mtklein wrote: > On 2015/11/24 at 20:45:28, halcanary wrote: > > On 2015/11/24 at 20:21:35, jcgregorio wrote: > > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > > > File experimental/fiddle/fiddler.go (right): > > > > > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > > > experimental/fiddle/fiddler.go:141: out, err := os.Create(path.Join(fiddle_dir, "skia.h")) > > > Generating skia.h should be part of cmake, that way options used in building the library can be baked into the skia.h header itself. > > > > You'll never convince mtklein@ of that. > > If you want to create a header file for user definitions, please use the existing SkUserConfig.h. We create a file ${CMAKE_BINARY_DIR}/include/SkUserConfig.h That's still outside of the scope of *fiddle*, which should just include everything.
On 2015/11/25 at 18:19:07, halcanary wrote: > On 2015/11/25 at 16:01:29, mtklein wrote: > > On 2015/11/24 at 20:45:28, halcanary wrote: > > > On 2015/11/24 at 20:21:35, jcgregorio wrote: > > > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > > > > File experimental/fiddle/fiddler.go (right): > > > > > > > > https://codereview.chromium.org/1349163003/diff/220001/experimental/fiddle/fi... > > > > experimental/fiddle/fiddler.go:141: out, err := os.Create(path.Join(fiddle_dir, "skia.h")) > > > > Generating skia.h should be part of cmake, that way options used in building the library can be baked into the skia.h header itself. > > > > > > You'll never convince mtklein@ of that. > > > > If you want to create a header file for user definitions, please use the existing SkUserConfig.h. > > We create a file ${CMAKE_BINARY_DIR}/include/SkUserConfig.h > > That's still outside of the scope of *fiddle*, which should just include everything. OK, so if that belongs in CMake configs, that belongs in CMake configs for fiddle (which depend on Skia's), not Skia's.
Okay, I now rely on CMake to create skia.h, SkUserConfig.h, skia_compile_arguments.txt, and skia_link_arguments.txt PTAL.
https://codereview.chromium.org/1349163003/diff/260001/experimental/fiddle/fi... File experimental/fiddle/fiddler.go (right): https://codereview.chromium.org/1349163003/diff/260001/experimental/fiddle/fi... experimental/fiddle/fiddler.go:40: glog.Fatalf("syscall.Setrlimit(RLIMIT_CPU) error: %v", err) glog.Fatal should really be restricted to being called in main(). So have setResourceLimits() return an error, let the caller decide if this is a fatal problem. Same comment applies to all glog.Fatal calls below. https://codereview.chromium.org/1349163003/diff/260001/experimental/fiddle/fi... experimental/fiddle/fiddler.go:91: // Compile the input Comments begin with the name of the thing being commented on: // fiddler compiles the input.
https://codereview.chromium.org/1349163003/diff/260001/experimental/fiddle/fi... File experimental/fiddle/fiddler.go (right): https://codereview.chromium.org/1349163003/diff/260001/experimental/fiddle/fi... experimental/fiddle/fiddler.go:40: glog.Fatalf("syscall.Setrlimit(RLIMIT_CPU) error: %v", err) On 2015/12/02 at 15:54:34, jcgregorio wrote: > glog.Fatal should really be restricted to being called in main(). So have setResourceLimits() return an error, let the caller decide if this is a fatal problem. > > Same comment applies to all glog.Fatal calls below. done https://codereview.chromium.org/1349163003/diff/260001/experimental/fiddle/fi... experimental/fiddle/fiddler.go:91: // Compile the input On 2015/12/02 at 15:54:34, jcgregorio wrote: > Comments begin with the name of the thing being commented on: > > // fiddler compiles the input. done
ping.
lgtm https://codereview.chromium.org/1349163003/diff/280001/experimental/fiddle/fi... File experimental/fiddle/fiddler.go (right): https://codereview.chromium.org/1349163003/diff/280001/experimental/fiddle/fi... experimental/fiddle/fiddler.go:150: defer func(closer io.Closer) { util.Close(inputFile)
On 2015/12/10 at 14:44:58, jcgregorio wrote: > lgtm > > https://codereview.chromium.org/1349163003/diff/280001/experimental/fiddle/fi... > File experimental/fiddle/fiddler.go (right): > > https://codereview.chromium.org/1349163003/diff/280001/experimental/fiddle/fi... > experimental/fiddle/fiddler.go:150: defer func(closer io.Closer) { > util.Close(inputFile) done
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jcgregorio@google.com Link to the patchset: https://codereview.chromium.org/1349163003/#ps300001 (title: "2015-12-10 (Thursday) 10:34:44 EST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349163003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349163003/300001
Message was sent while issue was closed.
Description was changed from ========== experiment/fiddle ========== to ========== experiment/fiddle Committed: https://skia.googlesource.com/skia/+/decb21e3ae3d296976d8664e49e35971d1b4fadd ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as https://skia.googlesource.com/skia/+/decb21e3ae3d296976d8664e49e35971d1b4fadd |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
